[Ovmsdev] Ioniq 5 - Pushing up some prerequisite changes to components/vehicles
Michael Balzer
dexter at expeedo.de
Sat Oct 29 22:48:32 HKT 2022
I've already merged PR #754 and commented on #753.
Other opinions on #753 are of course welcome.
Regards,
Michael
Am 29.10.22 um 16:25 schrieb Michael Geddes:
> No problems at all, these were just thoughts.
>
> So are those 2 PRs ok as they stand?
>
> I have a couple more in the pipeline(as I previously described)
> before the main I5 one.
>
> Mihael
>
>
> On Sat, 29 Oct 2022, 8:16 pm Michael Balzer, <dexter at expeedo.de> wrote:
>
> Michael,
>
> 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.
>
> 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.
>
> 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.
>
> Regards,
> Michael
>
>
> Am 27.10.22 um 09:36 schrieb 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 list
>>> OvmsDev at lists.openvehicles.com
>>> http://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
>>
>>
>> _______________________________________________
>> OvmsDev mailing list
>> OvmsDev at lists.openvehicles.com
>> http://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
>
>
> _______________________________________________
> OvmsDev mailing list
> OvmsDev at lists.openvehicles.com
> http://lists.openvehicles.com/mailman/listinfo/ovmsdev
--
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20221029/2134d016/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 203 bytes
Desc: OpenPGP digital signature
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20221029/2134d016/attachment-0001.sig>
More information about the OvmsDev
mailing list