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

[Ludovic LANGE]

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 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20221022/4cea598d/attachment.htm>