[Ovmsdev] Ioniq 5 - Pushing up some prerequisite changes to components/vehicles

[Michael Geddes]

While I'm still happy with the base data extraction tools, I'm wondering if
some more direct tools would be better.

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.

Or methods on the metrics themselves that has similar results.

I notice that index by 4 bit 'nibbles' is also a Thing That Is Done.

I'm happy to implement that if there's appetite for it... And possibly look
at a few Vehicles to switch to that


Michael

On Mon, 24 Oct 2022, 1:26 pm Michael Geddes, <frog at bunyip.wheelycreek.net>
wrote:

> 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.
> This means that the code to create the averages etc is just pulled out to
> a private function here:
> https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/753
>
> This P/R request adds the buffer extraction tools (with 32bit length) to
> ovms utils.
>
> https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/754
>
> //.ichael
>
> On Sun, 23 Oct 2022 at 17:38, Michael Geddes <frog at bunyip.wheelycreek.net>
> wrote:
>
>> Thanks Michael,
>>
>> 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.
>>
>>  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.
>> Hmm... yea - I guess I could do some dynamically created poll table.
>> Will definitely consider that.
>>
>> Further comments inline below:
>>
>>
>> On Sun, 23 Oct 2022 at 16:15, Michael Balzer <dexter at expeedo.de> wrote:
>>
>>> Michael,
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> So, besides the type independency, I think std::vector has no advantage
>>> here.
>>>
>>>
>>> There's another thing I'd like to address: it seems you're planning to
>>> use single polls extensively?
>>>
>>> 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).
>>>
>>> The single poll utility is really meant for sparse/manual polls outside
>>> the normal polling scheme. See my comment on the method:
>>>
>>>  * PollSingleRequest: perform *prioritized synchronous* single OBD2/UDS
>>> request
>>>  *  Pass a full OBD2/UDS request (mode/type, PID, additional payload).
>>>  *  The request is sent immediately, *aborting a running poll list
>>> request*. The previous
>>>  *  poller state is automatically restored after the request has been
>>> performed, but
>>>  *  *without any guarantee for repetition or omission of an aborted
>>> poll*.
>>>
>>> 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.
>>>
>>> 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.
>>>
>>>
>>> The place for general CAN utility methods is the
>>> components/can/src/canutils module, general utility methods go into the
>>> main/ovms_utils module.
>>>
>> Ahh.. cool.
>>
>>>
>>> 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.
>>>
>>> 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.
>>>
>>
>> I had already implemented little-endian and big-endian versions of the
>> data extract. I will now explicitly mark all of them.
>>
>>
>>>
>>> To be really general, your byte addressing needs to support more than
>>> 256 bytes in the buffer.
>>>
>> Consider that fixed.
>>
>>
>>> 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…
>>>
>> 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.
>> Something like: extract_int<1,2>(byte)  --> Extract bits len 2 starting
>> from bit 1. We can easily sign-extend that too.
>>
>>
>>> Regards,
>>> Michael
>>>
>>>
>>> Am 23.10.22 um 02:23 schrieb Michael Geddes:
>>>
>>> Hi Michael,
>>>
>>> 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).
>>>
>>> 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:
>>> https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/736
>>> - 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).
>>>
>>> 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.
>>>
>>> //.ichael
>>>
>>> On Sat, 22 Oct 2022 at 07:56, Michael Geddes <
>>> frog at bunyip.wheelycreek.net> wrote:
>>>
>>>> Thanks Michael,
>>>>
>>>> I had already created my commits under the assumption that you would
>>>> prefer the general changes separate from the vehicle-specific commits.
>>>>
>>>> I have created 3 pull requests so far.
>>>>   1) the bug - that's a no-brainer.
>>>>   2) 'finalisation' of temperatures and voltages. (Draft, see below)
>>>>   3) vector poll. This is more a point of discussion (see below)
>>>>
>>>> A couple of the next bugs are kind of dependent on the second, so I'll
>>>> wait until I get that sorted out.
>>>>
>>>> More comments below.
>>>>
>>>> On Fri, 21 Oct 2022 at 23:31, Michael Balzer <dexter at expeedo.de> wrote:
>>>>
>>>>> Michael,
>>>>>
>>>>> Am 19.10.22 um 07:26 schrieb Michael Geddes:
>>>>> > Hi all,
>>>>> >
>>>>> > I have 6 commits that I would _like_ to get pushed up that are
>>>>> changes
>>>>> > to components/vehicles required for my Ioniq 5 changes to work.
>>>>> >
>>>>> > I can push them all up in a single merge request for review, or
>>>>> > separate them out into bits.  Some of them I am expecting there to
>>>>> be
>>>>> > opinions on. I'm prepared to simplify some, and one in particular
>>>>> has
>>>>> > grounds for nixing. I think it's a good change, but I could manage
>>>>> > without it.
>>>>>
>>>>> Please keep PRs as small as possible, focusing on one specific
>>>>> modification / addition at a time. That way, every merge commit can
>>>>> easily be reverted if necessary, and changes can be followed and
>>>>> referred to easily. Especially keep framework changes separate from
>>>>> vehicle specific ones.
>>>>>
>>>>> It's also OK to open the PRs tagged as drafts / work in progress, that
>>>>> helps in commenting on certain parts and tracing refinements. Create
>>>>> one
>>>>> branch for each PR, pushing new commits into the branch will
>>>>> automatically update any open PR created from it.
>>>>
>>>> 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.
>>>>
>>>>
>>>>>
>>>>>
>>>> >
>>>>> > One is a simple bugfix - a literal one-character change:
>>>>> > void OvmsVehicle::BmsRestartCellTemperatures()
>>>>> >    {
>>>>> >    m_bms_bitset_t.clear();
>>>>> > -  m_bms_bitset_v.resize(m_bms_readings_t);
>>>>> > +  m_bms_bitset_t.resize(m_bms_readings_t);
>>>>> >    m_bms_bitset_ct = 0;
>>>>> >    }
>>>>>
>>>>> Obviously a copy & paste bug, nice find. I wonder how that could slip
>>>>> through without causing crashes, as there are more voltages than
>>>>> temperatures in most packs.
>>>>>
>>>>> Please don't hesitate with opening PRs even for single fixes like
>>>>> this,
>>>>> the sooner we include them, the better.
>>>>>
>>>>
>>>> Yep. Done and understood.
>>>>
>>>>
>>>>>
>>>>> >
>>>>> > On is a simple code readability change. Using an enum instead of
>>>>> magic
>>>>> > status values for m_bms_talerts.  Simple enough, but I'm prepared to
>>>>> > simplify that to a standard enum or get rid of it.
>>>>>
>>>>> The magic values are simply alert levels, but go ahead, readability is
>>>>> a
>>>>> good goal.
>>>>>
>>>>> I'll include it once the Battery Cell code is finalised.
>>>>
>>>>> >
>>>>> > There's some battery related ones where I want to be able to not
>>>>> quite
>>>>> > know the number of cells from the start and let the OBD responses
>>>>> specify.
>>>>>
>>>>> Dynamic pack layout reconfiguration is no issue, I've done that on the
>>>>> Twizy (look for comments "update pack layout" or calls to
>>>>> BmsSetCellArrangement*).
>>>>>
>>>>> 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.
>>>>
>>>> >
>>>>> > There's also a battery one where the info for the cells have
>>>>> different
>>>>> > timings (because one has other info), so I want to be able to clear
>>>>> a
>>>>> > range of cells rather than just reset and start filling the info
>>>>> again.
>>>>>
>>>>> That may be a bad idea, maybe better to implement some synchronization
>>>>> /
>>>>> drop readings out of sync.
>>>>>
>>>>> Reaching a consistent state, the BMS will calculate deviations etc..
>>>>> Crucial on that part is an analysis of the consistency of the cell
>>>>> readings to determine the reliability of deviations exceeding
>>>>> thresholds.
>>>>>
>>>>> See my comments in BmsSetCellVoltage() and list thread:
>>>>>
>>>>> http://lists.openvehicles.com/pipermail/ovmsdev/2021-February/015079.html
>>>>>
>>>>> 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.
>>>>
>>>>
>>>>> >
>>>>> > I've modified slightly the dump for the battery cell
>>>>> > voltage/temperature, allowing for either or both columns to be
>>>>> visible
>>>>> > if available. It also copes with the above 'not set' scenario.
>>>>>
>>>>> Sounds good.
>>>>>
>>>> I'll push this once the FinaliseCellVoltage pull-request is finalised
>>>> and accepted.
>>>>
>>>>>
>>>>> >
>>>>> > Finally, the controversial one:  I am using std::vector<uint8_t> as
>>>>> a
>>>>> > buffer in my ioniq5 code.. and I wanted to be able to use it for the
>>>>> > polling instead of std::string. I've not replaced the std::string
>>>>> but
>>>>> > added the ability to have either.
>>>>> >
>>>>> > If my aversion to using std::string for binary data is misplaced,
>>>>> then
>>>>> > I'm ok with the currently small modification to my code that would
>>>>> use
>>>>> > the std::string version.
>>>>>
>>>>> The general use case of ISO-TP data is actually dynamic binary
>>>>> strings.
>>>>> They are meant to and need to be easily aggregated, cut, merged,
>>>>> partially extracted etc., so they really are strings, not vectors
>>>>> (type
>>>>> wise).
>>>>
>>>> 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.
>>>>
>>>> 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.
>>>>
>>>> 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.
>>>>
>>>> The great thing about the template functions here is that they should
>>>> expand inline at compile time to quite efficient code.
>>>>
>>>>>
>>>>> Regards,
>>>>> Michael
>>>>>
>>>> //.ichael
>>>>
>>>
>>> _______________________________________________
>>> OvmsDev mailing listOvmsDev at lists.openvehicles.comhttp://lists.openvehicles.com/mailman/listinfo/ovmsdev
>>>
>>>
>>> --
>>> Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
>>> Fon 02333 / 833 5735 * Handy 0176 / 206 989 26
>>>
>>> _______________________________________________
>>> OvmsDev mailing list
>>> OvmsDev at lists.openvehicles.com
>>> http://lists.openvehicles.com/mailman/listinfo/ovmsdev
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20221027/a56bff35/attachment-0001.htm>