<div dir="auto"><div>While I'm still happy with the base data extraction tools, I'm wondering if some more direct tools would be better. </div><div dir="auto"><br></div><div dir="auto">For example some methods on the Base vehicle that takes a buffer, a byte index and template byte count a Metric object and a multiplier.. That will conditionally set the metric if the data is within bounds. </div><div dir="auto"><br></div><div dir="auto">Or methods on the metrics themselves that has similar results. </div><div dir="auto"><br></div><div dir="auto">I notice that index by 4 bit 'nibbles' is also a Thing That Is Done. </div><div dir="auto"><br></div><div dir="auto">I'm happy to implement that if there's appetite for it... And possibly look at a few Vehicles to switch to that</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto">Michael<br><br><div class="gmail_quote" dir="auto"><div dir="ltr" class="gmail_attr">On Mon, 24 Oct 2022, 1:26 pm Michael Geddes, <<a href="mailto:frog@bunyip.wheelycreek.net">frog@bunyip.wheelycreek.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Ok - so I've pared down my requirements on the BMS module to the ability to be able to cap the # of Voltages/Temperatures at a lower (only) value than initialised at.<br></div><div>This means that the code to create the averages etc is just pulled out to a private function here:</div><a href="https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/753" target="_blank" rel="noreferrer">https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/753</a><div><br></div><div>This P/R request adds the buffer extraction tools (with 32bit length) to ovms utils.<br><div><br></div><div><a href="https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/754" target="_blank" rel="noreferrer">https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/754</a><br><div><br></div><div>//.ichael</div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 23 Oct 2022 at 17:38, Michael Geddes <<a href="mailto:frog@bunyip.wheelycreek.net" target="_blank" rel="noreferrer">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,<br><div><br></div><div>I have taken it in, and I'll use std::string in poll once code. I guess it might make sense to convert everything else to std::string. It wouldn't be a big task.</div><div><br></div><div> I had just taken out that commit and reworked anyway, and now that you say it's not the best way of doing things, I'm even happier I made that choice, as I had intended to possibly make more use of it and won't now. At the moment I'm using it to get the VIN (snipped) where I'll try a couple of times only, rather than continually poll for it.<br></div><div>Hmm... yea - I guess I could do some dynamically created poll table. Will definitely consider that.</div><div><br></div><div>Further comments inline below:</div><div> </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 23 Oct 2022 at 16:15, Michael Balzer <<a href="mailto:dexter@expeedo.de" target="_blank" rel="noreferrer">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">
<div>
Michael,<br>
<br>
what I really dislike about your change is that it adds complexity
to the underlying function just to fulfill an -- in my eyes more
academic -- type style choice: having to handle multiple buffer
pointers just to provide different typing approaches is bad. You
could introduce a new construct to provide a single polymorphic
pointer, but that again adds complexity.<br>
<br>
Agreed, std::string is a bit type-sloppy in depending on char being
unsigned 8 bit, but that's all to consider, and can easily be
enforced if ever necessary.<br>
<br>
On the plus side, std::string has far better library support, better
interoperability (see our use cases), and small string optimization,
which is especially useful for our platform, as most poll requests
and responses easily fit into the initial capacity, normally
eliminating the need for heap allocations.<br>
<br>
So, besides the type independency, I think std::vector has no
advantage here.<br>
<br>
<br>
There's another thing I'd like to address: it seems you're planning
to use single polls extensively?<br>
<br>
If so, please don't, unless you're also going to do a full rewrite
of the poller towards a job/worker service task architecture (that's
a very old todo).<br>
<br>
The single poll utility is really meant for sparse/manual polls
outside the normal polling scheme. See my comment on the method:<br>
<br>
<font face="monospace"> * PollSingleRequest: perform <b>prioritized
synchronous</b> single OBD2/UDS request<br>
* Pass a full OBD2/UDS request (mode/type, PID, additional
payload).<br>
* The request is sent immediately, <b>aborting a running poll
list request</b>. The previous<br>
* poller state is automatically restored after the request has
been performed, but<br>
* <b>without any guarantee for repetition or omission of an
aborted poll</b>.</font><br>
<br>
This was really meant more for user/script use than for standard
vehicle polls, though it's OK to use it, where applicable, with
caution.<br>
<br>
For all standard polling, install a PID list. You can vary a list by
the poll state, and you can switch lists if necessary. PID lists can
easily be generated dynamically, see the VWUP code for an example.<br>
<br>
<br>
The place for general CAN utility methods is the
components/can/src/canutils module, general utility methods go into
the main/ovms_utils module.<br></div></blockquote><div>Ahh.. cool. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
<br>
Regarding CAN buffer data extraction, there are currently probably
as many approaches as there are vehicle modules. Certainly would be
nice to have some standard way here, but won't be simple to rework
the existing vehicle adaptors.<br>
<br>
The template approach is a nice one for a general solution, but keep
in mind not every vehicle will use big endian encoding, in some
cases not even consistently throughout the installed devices.<br></div></blockquote><div><br></div><div>I had already implemented little-endian and big-endian versions of the data extract. I will now explicitly mark all of them.</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"><div>
<br>
To be really general, your byte addressing needs to support more
than 256 bytes in the buffer. </div></blockquote><div>Consider that fixed.</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"><div>Your templates also lack bit masking
and shifting. Some devices use packed structs with odd sized bit
fields. The DBC engine has support for all this, but not
generalized. I remember Mark getting a headache from this…<br></div></blockquote><div>I have a get_bit<>() function and so I would propose it be implemented in a similar way... ie exract the byte first, then get the bits.</div><div>Something like: extract_int<1,2>(byte) --> Extract bits len 2 starting from bit 1. We can easily sign-extend that too.</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"><div>
Regards,<br>
Michael<br>
<br>
<br>
<div>Am 23.10.22 um 02:23 schrieb Michael
Geddes:<br>
</div>
<blockquote type="cite">
<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" target="_blank" rel="noreferrer">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" target="_blank" rel="noreferrer">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" rel="noreferrer">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 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>
<br>
<fieldset></fieldset>
<pre>_______________________________________________
OvmsDev mailing list
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank" rel="noreferrer">OvmsDev@lists.openvehicles.com</a>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" target="_blank" rel="noreferrer">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a>
</pre>
</blockquote>
<br>
<pre cols="72">--
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26</pre>
</div>
_______________________________________________<br>
OvmsDev mailing list<br>
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank" rel="noreferrer">OvmsDev@lists.openvehicles.com</a><br>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" rel="noreferrer noreferrer" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a><br>
</blockquote></div></div>
</blockquote></div>
</blockquote></div></div></div>