<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
Michael,<br>
<br>
please don't change existing vehicle code for personal code style
reasons. The existing data extraction methods have been working and
will continue to work. By adding your utilities to the framework,
we're giving everyone interested the opportunity to consider using
these, we don't force them to do.<br>
<br>
Regarding your combined data extraction & metric setting
utility, from my experience this won't be applicable in many (most)
cases, or would need quite a lot of extra features exceeding a
simple scaling. Applications often need or want to change metrics
only indirectly after processing the raw data read from the bus,
often in combination with other readings and/or delayed. Validity
checks also need to be done in a broad variety, often with side
conditions.<br>
<br>
A good approach is to first define & refine your generalized
methods locally in your component. Keep an eye on the existing
method signatures, so once you're happy with them it's easy to move
them into the framework. That way you'll avoid having to break
compatibility for other developers possibly already using an early
release of your method.<br>
<br>
Regards,<br>
Michael<br>
<br>
<br>
<div class="moz-cite-prefix">Am 27.10.22 um 09:36 schrieb Michael
Geddes:<br>
</div>
<blockquote type="cite"
cite="mid:CAH0p7u+afKb36v_uUqTLJ8BG2GvPdA9KzkurNxh4L0cFJsi_gQ@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<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"
moz-do-not-send="true" class="moz-txt-link-freetext">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"
moz-do-not-send="true" class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true" class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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" moz-do-not-send="true" class="moz-txt-link-freetext">OvmsDev@lists.openvehicles.com</a>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" target="_blank" rel="noreferrer" moz-do-not-send="true" class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">OvmsDev@lists.openvehicles.com</a><br>
<a
href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev"
rel="noreferrer noreferrer" target="_blank"
moz-do-not-send="true"
class="moz-txt-link-freetext">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a><br>
</blockquote>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</div>
<br>
<fieldset class="moz-mime-attachment-header"></fieldset>
<pre class="moz-quote-pre" wrap="">_______________________________________________
OvmsDev mailing list
<a class="moz-txt-link-abbreviated" href="mailto:OvmsDev@lists.openvehicles.com">OvmsDev@lists.openvehicles.com</a>
<a class="moz-txt-link-freetext" href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a>
</pre>
</blockquote>
<br>
<pre class="moz-signature" cols="72">--
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26</pre>
</body>
</html>