<div dir="ltr">Commented.<div>However, I also just realised that the code would permit </div><div><span style="font-family:Consolas,"Courier New",monospace;font-size:14px;white-space:pre-wrap;background-color:rgb(30,30,30);color:rgb(220,220,170)">sign_extend</span><span style="color:rgb(212,212,212);font-family:Consolas,"Courier New",monospace;font-size:14px;white-space:pre-wrap;background-color:rgb(30,30,30)"><</span><span style="font-family:Consolas,"Courier New",monospace;font-size:14px;white-space:pre-wrap;background-color:rgb(30,30,30);color:rgb(78,201,176)">uint32_t</span><span style="color:rgb(212,212,212);font-family:Consolas,"Courier New",monospace;font-size:14px;white-space:pre-wrap;background-color:rgb(30,30,30)">, u</span><span style="font-family:Consolas,"Courier New",monospace;font-size:14px;white-space:pre-wrap;background-color:rgb(30,30,30);color:rgb(78,201,176)">int32_t</span><span style="color:rgb(212,212,212);font-family:Consolas,"Courier New",monospace;font-size:14px;white-space:pre-wrap;background-color:rgb(30,30,30)">></span><br></div><div>thus avoid any extra casts!</div><div>Obviously an unusual case for this; being cast back and forth .. but really it won't do anything to generated binary!</div><div><br></div><div>I also suggest that we find a .h file to put this sign_extend function into, which is beyond my pay grade.</div><div><br></div><div>//.ichael</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 22 Oct 2022 at 21:49, Michael Geddes <<a href="mailto:frog@bunyip.wheelycreek.net">frog@bunyip.wheelycreek.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">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.<div><br></div><div>I could possibly have made this a little better.. but it wouldn't have changed the code generation much.</div><div>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. </div><div><div style="color:rgb(212,212,212);background-color:rgb(30,30,30);font-family:Consolas,"Courier New",monospace;font-size:14px;line-height:19px;white-space:pre-wrap"><br><div><span style="color:rgb(106,153,85)">/** Sign extend an unsigned to a signed integer of the same size.</span></div><div><span style="color:rgb(106,153,85)"> */</span></div><div><span style="color:rgb(86,156,214)">template</span><<span style="color:rgb(86,156,214)">typename</span> <span style="color:rgb(78,201,176)">UINT</span>, <span style="color:rgb(86,156,214)">typename</span> <span style="color:rgb(78,201,176)">INT</span>></div><div><span style="color:rgb(78,201,176)">INT</span> <span style="color:rgb(220,220,170)">sign_extend</span>( <span style="color:rgb(78,201,176)">UINT</span> <span style="color:rgb(156,220,254)">uvalue</span>, <span style="color:rgb(78,201,176)">uint8_t</span> <span style="color:rgb(156,220,254)">signbit</span>)</div><div>{</div><div> <span style="color:rgb(86,156,214)">typedef</span> <span style="color:rgb(86,156,214)">typename</span> <span style="color:rgb(78,201,176)">std</span>::<span style="color:rgb(78,201,176)">make_unsigned</span><<span style="color:rgb(78,201,176)">INT</span>>::<span style="color:rgb(78,201,176)">type</span> <span style="color:rgb(78,201,176)">uint_t</span>;</div><div> <span style="color:rgb(78,201,176)">uint_t</span> <span style="color:rgb(156,220,254)">newuvalue</span> = <span style="color:rgb(156,220,254)">uvalue</span>;</div><div> <span style="color:rgb(197,134,192)">if</span> ( <span style="color:rgb(156,220,254)">newuvalue</span> & ( <span style="color:rgb(78,201,176)">UINT</span>(<span style="color:rgb(181,206,168)">1U</span>) << <span style="color:rgb(156,220,254)">signbit</span>)) ) {</div><div> newuvalue |= ~((<span style="color:rgb(78,201,176)">uint_t</span>(<span style="color:rgb(181,206,168)">1U</span>) << signbit)) - <span style="color:rgb(181,206,168)">1</span>);</div><div> }</div><div> <span style="color:rgb(197,134,192)">return</span> <span style="color:rgb(86,156,214)">reinterpret_cast</span><INT &>(uvalue);</div><div>}</div></div></div><div><br></div><div>It would be possible to have both define and use the one that suits!<br></div><div>then the code would become:</div><div><br></div><div><div style="color:rgb(212,212,212);background-color:rgb(30,30,30);font-family:Consolas,"Courier New",monospace;font-size:14px;line-height:19px;white-space:pre-wrap"><div><span style="color:rgb(197,134,192)"> if</span> (<span style="color:rgb(156,220,254)">m_value_type</span> == <span style="color:rgb(79,193,255)">DBC_VALUETYPE_UNSIGNED</span>)</div><div> <span style="color:rgb(156,220,254)">result</span>.<span style="color:rgb(220,220,170)">Cast</span>((<span style="color:rgb(78,201,176)">uint32_t</span>)<span style="color:rgb(156,220,254)">val</span>, <span style="color:rgb(79,193,255)">DBC_NUMBER_INTEGER_UNSIGNED</span>);</div><div> <span style="color:rgb(197,134,192)">else</span></div><div> {</div><div> <span style="color:rgb(78,201,176)">int32_t</span> <span style="color:rgb(156,220,254)">iVal</span> = <span style="color:rgb(220,220,170)">sign_extend</span><<span style="color:rgb(78,201,176)">uint32_t</span>, <span style="color:rgb(78,201,176)">int32_t</span>>((<span style="color:rgb(78,201,176)">uint32_t</span>)<span style="color:rgb(156,220,254)">val</span>,<span style="color:rgb(156,220,254)">m_signal_size</span>-<span style="color:rgb(181,206,168)">1</span>);</div><div> <span style="color:rgb(156,220,254)">result</span>.<span style="color:rgb(220,220,170)">Cast</span>(<span style="color:rgb(86,156,214)">static_cast</span><<span style="color:rgb(78,201,176)">uint32_t</span>>(<span style="color:rgb(156,220,254)">iVal</span>), <span style="color:rgb(79,193,255)">DBC_NUMBER_INTEGER_SIGNED</span>);</div><div> }</div></div></div></div><div><br></div><div>in the sign_extend parameters, it is designed to be able to have the unsigned and signed types as different sizes.</div><div>For example</div><div>sign_extend<uint8_t, int64_t>(val, signbit);</div><div>This would sign-extend the 8bit unsigned input into 64 bit signed.</div><div><br></div><div>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.</div><div><br></div><div>//.ichael</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 22 Oct 2022 at 21:08, Ludovic LANGE <<a href="mailto:ll-ovmsdev@lange.nom.fr" target="_blank">ll-ovmsdev@lange.nom.fr</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<div>Hi Michael,</div>
<div><br>
</div>
<div>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.<br>
</div>
<div><br>
</div>
<div>Just if you have a few minutes to
enlighten me : if we were to look at the proposed patch here
<a href="https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/736/files" target="_blank">https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/736/files</a>
; and if we would introduce your template implementation, how
should we rewrite the "patched" version to make use of this
implementation ?<br>
</div>
<blockquote>
<div><span>result.<span>Cast</span>((<span>uint32_t</span>)<span>SIGNEX</span><span>(</span>val<span>, m_signal_size-</span><span>1</span><span>)</span>,
DBC_NUMBER_INTEGER_SIGNED);</span></div>
</blockquote>
<div>(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)<br>
</div>
<div><br>
</div>
<div>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 ?</div>
<div><br>
</div>
<div>Sorry for being ignorant on this
subject.</div>
<div><br>
</div>
<div>Regards,</div>
<div><br>
</div>
<div>Ludovic<br>
</div>
<div><br>
</div>
<div><br>
</div>
<div>Le 14/10/2022 à 10:40, Michael Geddes a
écrit :<br>
</div>
<blockquote type="cite">
<div dir="ltr">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:<br>
<div>/** Sign extend an unsigned to a signed integer of the same
size.<br>
*/<br>
template<typename UINT, typename INT, uint8_t SIGNBIT><br>
INT sign_extend( UINT uvalue)<br>
{<br>
typedef typename std::make_unsigned<INT>::type uint_t;<br>
uint_t newuvalue = uvalue;<br>
if ( newuvalue & ( UINT(1U) << SIGNBIT) ) {<br>
newuvalue |= ~((uint_t(1U) << SIGNBIT) - 1);<br>
}<br>
return reinterpret_cast<INT &>(uvalue);<br>
}<br>
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.</div>
<div>Usage from my code: Where BYTES is a templated integer
value parameter.</div>
<div> res = sign_extend < uint32_t, int32_t, BYTES * 8 - 1
> (ures);<br>
</div>
<div><br>
</div>
<div>I also have a templated buffer extractor that allows for
checking available space!</div>
<div>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:</div>
<div><br>
</div>
int32_t signedValue;<br>
if (!can_buff_int<2>(data, 10, signedValue )) {<br>
ESP_LOGE(TAG, "IoniqISOTP.BMC: BMS Current: Bad
Buffer");<br>
} else {<br>
StdMetrics.ms_v_bat_current->SetValue((float)signedValue /
10.0, Amps);<br>
<div> } </div>
<div><br>
</div>
<div>//.ichael</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Fri, 14 Oct 2022 at 05:35,
Ludovic LANGE <<a href="mailto:ll-ovmsdev@lange.nom.fr" target="_blank">ll-ovmsdev@lange.nom.fr</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p>Dear list,</p>
<p>I've come across an issue with the DBC parser where a
non-32-bit negative number was incorrectly decoded.</p>
<p>I've traced this to an issue with sign-extension ; and
I've proposed a fix here
<a href="https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/736" target="_blank">https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/736</a></p>
<p>Before having it merged, I'd like to gather some feedback
from users of the DBC module.</p>
<p>Please have a look, and comment directly on the GitHub
PR.</p>
<p>Thanks in advance !</p>
<p>Regards,</p>
<p>Ludovic<br>
</p>
</div>
_______________________________________________<br>
OvmsDev mailing list<br>
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank">OvmsDev@lists.openvehicles.com</a><br>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" rel="noreferrer" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a><br>
</blockquote>
</div>
<br>
<fieldset></fieldset>
<pre>_______________________________________________
OvmsDev mailing list
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank">OvmsDev@lists.openvehicles.com</a>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a>
</pre>
</blockquote>
<p><br>
</p>
<div id="m_6523285523402742266m_-7229894858775450088grammalecte_menu_main_button_shadow_host" style="width:0px;height:0px"></div>
</div>
_______________________________________________<br>
OvmsDev mailing list<br>
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank">OvmsDev@lists.openvehicles.com</a><br>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" rel="noreferrer" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a><br>
</blockquote></div></div>
</blockquote></div>