<div dir="ltr">Hi Michael,<div><br></div><div>Just wanted to say that if you override my objections on the std::vector<unit8_t> in the single poll code then I'm fine with that, I recognised that it's absolutely your call and that bit has a small impact on my code so far.   I can even add versions of the extraction functions that use std::string if you want (they would just be inline template wrappers around common code anyway so zero compiled cost).  </div><div><br></div><div>Also: Is there a better place to put those templated functions?  A version of my sign_extend function is already being proposed being used here: <a href="https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/736">https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/736</a> - it would be good if both versions of this function were available in a single place.  (There's   sign_extend<UINT, INT>(int_value, sign_bit)  and  sign_extend<UINT, INT, SIGNBIT>(int_value)  - so they should work fine as overloaded template functions).</div><div><br></div><div>I would consider also whether I use std::string in my own code where I'm extracting my data out of complete ISOTP poll results. Up to you. That has slightly more impact, but the code change is pretty easy.</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 07:56, 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">Thanks Michael,</div><div dir="ltr"><br><div>I had already created my commits under the assumption that you would prefer the general changes separate from the vehicle-specific commits.</div><div><br><div>I have created 3 pull requests so far.</div><div>  1) the bug - that's a no-brainer.  </div><div>  2) 'finalisation' of temperatures and voltages. (Draft, see below)</div><div>  3) vector poll. This is more a point of discussion (see below)</div><div><br></div><div>A couple of the next bugs are kind of dependent on the second, so I'll wait until I get that sorted out.</div></div><div><br></div><div>More comments below.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 21 Oct 2022 at 23:31, Michael Balzer <<a href="mailto:dexter@expeedo.de" target="_blank">dexter@expeedo.de</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">Michael,<br>
<br>
Am 19.10.22 um 07:26 schrieb Michael Geddes:<br>
> Hi all,<br>
><br>
> I have 6 commits that I would _like_ to get pushed up that are changes <br>
> to components/vehicles required for my Ioniq 5 changes to work.<br>
><br>
> I can push them all up in a single merge request for review, or <br>
> separate them out into bits.  Some of them I am expecting there to be <br>
> opinions on. I'm prepared to simplify some, and one in particular has <br>
> grounds for nixing. I think it's a good change, but I could manage <br>
> without it.<br>
<br>
Please keep PRs as small as possible, focusing on one specific <br>
modification / addition at a time. That way, every merge commit can <br>
easily be reverted if necessary, and changes can be followed and <br>
referred to easily. Especially keep framework changes separate from <br>
vehicle specific ones.<br>
<br>
It's also OK to open the PRs tagged as drafts / work in progress, that <br>
helps in commenting on certain parts and tracing refinements. Create one <br>
branch for each PR, pushing new commits into the branch will <br>
automatically update any open PR created from it.</blockquote><div>Ooh. That's useful. It's been a while since I've used github for collab. Switched the battery sensor P/R over to a draft.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> One is a simple bugfix - a literal one-character change:<br>
> void OvmsVehicle::BmsRestartCellTemperatures()<br>
>    {<br>
>    m_bms_bitset_t.clear();<br>
> -  m_bms_bitset_v.resize(m_bms_readings_t);<br>
> +  m_bms_bitset_t.resize(m_bms_readings_t);<br>
>    m_bms_bitset_ct = 0;<br>
>    }<br>
<br>
Obviously a copy & paste bug, nice find. I wonder how that could slip <br>
through without causing crashes, as there are more voltages than <br>
temperatures in most packs.<br>
<br>
Please don't hesitate with opening PRs even for single fixes like this, <br>
the sooner we include them, the better.<br></blockquote><div><br></div><div>Yep. Done and understood. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
><br>
> On is a simple code readability change. Using an enum instead of magic <br>
> status values for m_bms_talerts.  Simple enough, but I'm prepared to <br>
> simplify that to a standard enum or get rid of it.<br>
<br>
The magic values are simply alert levels, but go ahead, readability is a <br>
good goal.<br>
<br></blockquote><div>I'll include it once the Battery Cell code is finalised. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> There's some battery related ones where I want to be able to not quite <br>
> know the number of cells from the start and let the OBD responses specify.<br>
<br>
Dynamic pack layout reconfiguration is no issue, I've done that on the <br>
Twizy (look for comments "update pack layout" or calls to <br>
BmsSetCellArrangement*).<br>
<br></blockquote><div>BmsSetCellArrangement does clear all the values... which is probably the main thing I was trying to avoid.  Ie- I get to the end and go 'ooh.. only this number of cells were filled in'.  More comments in the P/R about what I want to achieve and fall-back positions.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> There's also a battery one where the info for the cells have different <br>
> timings (because one has other info), so I want to be able to clear a <br>
> range of cells rather than just reset and start filling the info again.<br>
<br>
That may be a bad idea, maybe better to implement some synchronization / <br>
drop readings out of sync.<br>
<br>
Reaching a consistent state, the BMS will calculate deviations etc.. <br>
Crucial on that part is an analysis of the consistency of the cell <br>
readings to determine the reliability of deviations exceeding thresholds.<br>
<br>
See my comments in BmsSetCellVoltage() and list thread: <br>
<a href="http://lists.openvehicles.com/pipermail/ovmsdev/2021-February/015079.html" rel="noreferrer" target="_blank">http://lists.openvehicles.com/pipermail/ovmsdev/2021-February/015079.html</a><br>
<br></blockquote><div>I've skimmed the comments but will read properly and digest.   Yeah it might be best if I just clear the values at the start of the more often running one and then once the other ones come in it will finalise once it gets to the end of those.. so they aren't too far apart.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> I've modified slightly the dump for the battery cell <br>
> voltage/temperature, allowing for either or both columns to be visible <br>
> if available. It also copes with the above 'not set' scenario.<br>
<br>
Sounds good.<br></blockquote><div>I'll push this once the FinaliseCellVoltage pull-request is finalised and accepted.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> Finally, the controversial one:  I am using std::vector<uint8_t> as a <br>
> buffer in my ioniq5 code.. and I wanted to be able to use it for the <br>
> polling instead of std::string. I've not replaced the std::string but <br>
> added the ability to have either.<br>
><br>
> If my aversion to using std::string for binary data is misplaced, then <br>
> I'm ok with the currently small modification to my code that would use <br>
> the std::string version.<br>
<br>
The general use case of ISO-TP data is actually dynamic binary strings. <br>
They are meant to and need to be easily aggregated, cut, merged, <br>
partially extracted etc., so they really are strings, not vectors (type <br>
wise).</blockquote><div>Hmm.. I'm not sure I entirely agree about needing to aggregate/cut/merge; we know the size of the buffer after the first packet comes in so we can pre-set the buffer internal size and append data as it comes in without casting from uint8_t* to char *.  Even if the size is wrong, we can still very easily append to a vector.</div><div><br></div><div>ON the extraction, I have efficient templated functions that extract the data from the std::vector<uint8_t> buffer.. including to a std::string if required, or to big-endian/little-endian signed/unsigned integers of 1..4 bytes with sign-extension now.  These also check bounds so are quite safe - possibly safer than the macros.  The bounds check has already proved itself useful.</div><div><br></div><div>I have pushed up a draft  request 'single poll to buffer' as it is the easiest way to describe what I want to do.  The first commit is the meat of the change ... but the second is adding all the utility functions.  They probably don't belong in vehicle.h/cpp, but that's easy enough to change.  I had them in my ioniq 5 specific code - but have shoved them in there just for a point of discussion.</div><div><br></div><div>The great thing about the template functions here is that they should expand inline at compile time to quite efficient code.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Regards,<br>
Michael<br></blockquote><div>//.ichael </div></div></div>
</blockquote></div>