DBC : proposal fix for negative integer numbers having wrong values
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
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@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
Hi Michael, Thank you a lot for your message... however I must admit my complete 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/fi... ; 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@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
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@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/fi... ; 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@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________ OvmsDev mailing listOvmsDev@lists.openvehicles.comhttp://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
Hello Michael, Thanks for taking the time to help me understand that. So I've reworked the PR here https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/736 with your implementation (and added you as a co-author if you are OK) which seems to work as expected (I changed a few things to make it work). If you (and/or anybody else who is using DBC...) can review and approve this final version, we would then be ready for a merge. Thanks again, Le 22/10/2022 à 15:49, Michael Geddes a écrit :
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@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/fi... ; 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
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@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@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/fi... ; 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@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________ OvmsDev mailing listOvmsDev@lists.openvehicles.comhttp://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
participants (2)
-
Ludovic LANGE -
Michael Geddes