[Ovmsdev] DBC : proposal fix for negative integer numbers having wrong values

[Michael Geddes]

The other benefit of templating rather than macros is that macros will
insert the code as it is in the parameter, potentially causing some
unnecessary execution of code, or unwanted side effects, where as with a
template function, you still get the benefit of code expansion, but the
parameters are evaluated and then used in the inline expanded function.

I could possibly have made this a little better.. but it wouldn't have
changed the code generation much.
Ahh.. so in my use case, the sign-bin is templated (fixed at compile time),
which would mean that all that shifting arithmetic gets done at
compile-time... but in your case, we would need to have it as a parameter.

/** Sign extend an unsigned to a signed integer of the same size.
 */
template<typename UINT, typename INT>
INT sign_extend( UINT uvalue, uint8_t signbit)
{
  typedef typename std::make_unsigned<INT>::type uint_t;
  uint_t newuvalue = uvalue;
  if ( newuvalue & ( UINT(1U) << signbit)) ) {
    newuvalue |= ~((uint_t(1U) << signbit)) - 1);
  }
  return reinterpret_cast<INT &>(uvalue);
}

It would be possible to have both define and use the one that suits!
then the code would become:

if (m_value_type == DBC_VALUETYPE_UNSIGNED)
    result.Cast((uint32_t)val, DBC_NUMBER_INTEGER_UNSIGNED);
  else
    {
    int32_t iVal = sign_extend<uint32_t, int32_t>((uint32_t)val,
m_signal_size-1);
    result.Cast(static_cast<uint32_t>(iVal), DBC_NUMBER_INTEGER_SIGNED);
    }

in the sign_extend parameters, it is designed to be able to have the
unsigned and signed types as different sizes.
For example
sign_extend<uint8_t, int64_t>(val, signbit);
This would sign-extend the 8bit unsigned input into 64 bit signed.

YOu can use your old code.. but be aware that (m_signal_size-1) would be
copied  out ,and evaluated twice! Rather than evaluated once.

//.ichael

On Sat, 22 Oct 2022 at 21:08, Ludovic LANGE <ll-ovmsdev at lange.nom.fr> wrote:

> Hi Michael,
>
> Thank you a lot for your message... however I must admit my
> co//.ichaemplete ignorance (and lack of practice) of the whole C++ /
> templating world. I mean that I cannot understand (yet) your template
> version - such a shame ; nor adapt it to this fix.
>
> Just if you have a few minutes to enlighten me : if we were to look at the
> proposed patch here
> https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/736/files
> ; and if we would introduce your template implementation, how should we
> rewrite the "patched" version to make use of this implementation ?
>
> result.Cast((uint32_t)SIGNEX(val, m_signal_size-1),
> DBC_NUMBER_INTEGER_SIGNED);
>
> (my naïve attempts all failed compilation with all sorts of "error: use of
> 'this' in a constant expression" and "error: no matching function for call
> to 'sign_extend(uint64_t&)'" so a little help would be appreciated)
>
> Additionally, could you please expand a little on the benefits it adds -
> you're talking about type-safety, if you could just illustrate it in this
> context ?
>
> Sorry for being ignorant on this subject.
>
> Regards,
>
> Ludovic
>
>
> Le 14/10/2022 à 10:40, Michael Geddes a écrit :
>
> OOH.. nice one. I had missed that in my template implementation.  I'm a
> big fan of using C++ templating to do type-safe versions of this kind of
> stuff...   This is my template version:
> /** Sign extend an unsigned to a signed integer of the same size.
>  */
> template<typename UINT, typename INT, uint8_t SIGNBIT>
> INT sign_extend( UINT uvalue)
> {
>   typedef typename std::make_unsigned<INT>::type uint_t;
>   uint_t newuvalue = uvalue;
>   if ( newuvalue & ( UINT(1U) << SIGNBIT) ) {
>     newuvalue |= ~((uint_t(1U) << SIGNBIT) - 1);
>   }
>   return reinterpret_cast<INT &>(uvalue);
> }
> It could probably be converted into a version that would infer some of the
> types.. but this should work pretty well and should compile down to close
> to the same code.
> Usage from my code: Where BYTES is a templated integer value parameter.
>  res = sign_extend < uint32_t, int32_t, BYTES * 8 - 1 > (ures);
>
> I also have a templated buffer extractor that allows for checking
> available space!
> For example this code (which extracts 2 bytes as a signed integer), which
> should now work with sign extension thanks to your bugfix and the above
> templated version of sign extension:
>
>          int32_t signedValue;
>          if (!can_buff_int<2>(data, 10, signedValue )) {
>            ESP_LOGE(TAG, "IoniqISOTP.BMC: BMS Current: Bad Buffer");
>          } else {
>            StdMetrics.ms_v_bat_current->SetValue((float)signedValue /
> 10.0, Amps);
>          }
>
> //.ichael
>
>
>
>
> On Fri, 14 Oct 2022 at 05:35, Ludovic LANGE <ll-ovmsdev at lange.nom.fr>
> wrote:
>
>> Dear list,
>>
>> I've come across an issue with the DBC parser where a non-32-bit negative
>> number was incorrectly decoded.
>>
>> I've traced this to an issue with sign-extension ; and I've proposed a
>> fix here
>> https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/736
>>
>> Before having it merged, I'd like to gather some feedback from users of
>> the DBC module.
>>
>> Please have a look, and comment directly on the GitHub PR.
>>
>> Thanks in advance !
>>
>> Regards,
>>
>> Ludovic
>> _______________________________________________
>> OvmsDev mailing list
>> OvmsDev at lists.openvehicles.com
>> http://lists.openvehicles.com/mailman/listinfo/ovmsdev
>>
>
> _______________________________________________
> OvmsDev mailing listOvmsDev at lists.openvehicles.comhttp://lists.openvehicles.com/mailman/listinfo/ovmsdev
>
>
> _______________________________________________
> OvmsDev mailing list
> OvmsDev at lists.openvehicles.com
> http://lists.openvehicles.com/mailman/listinfo/ovmsdev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20221022/1ee6431a/attachment-0001.htm>