[Ovmsdev] DBC : proposal fix for negative integer numbers having wrong values
Michael Geddes
frog at bunyip.wheelycreek.net
Sun Oct 23 08:03:35 HKT 2022
Commented.
However, I also just realised that the code would permit
sign_extend<uint32_t, uint32_t>
thus avoid any extra casts!
Obviously an unusual case for this; being cast back and forth .. but
really it won't do anything to generated binary!
I also suggest that we find a .h file to put this sign_extend function
into, which is beyond my pay grade.
//.ichael
On Sat, 22 Oct 2022 at 21:49, Michael Geddes <frog at bunyip.wheelycreek.net>
wrote:
> 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/20221023/66eba99c/attachment-0001.htm>
More information about the OvmsDev
mailing list