[Ovmsdev] Ioniq 5 - Pushing up some prerequisite changes to components/vehicles
Michael Geddes
frog at bunyip.wheelycreek.net
Mon Oct 24 13:26:52 HKT 2022
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/20221024/47107915/attachment-0001.htm>
More information about the OvmsDev
mailing list