[Ovmsdev] Ioniq 5 - Pushing up some prerequisite changes to components/vehicles
Michael Balzer
dexter at expeedo.de
Wed Nov 2 14:57:22 HKT 2022
If you only add the #defines, that's ok included in your PR. If you also
change usages of 0x7df etc. in the vehicle framework, that would be a
separate PR.
Regards,
Michael
Am 02.11.22 um 00:54 schrieb Michael Geddes:
> I'm close to the ioniq5 release now. (Read-only for the moment).
> Thanks for your support and patience!
>
> One of the things I included were the following defines in vehicle.h
> which I think should go here, but I'm really not fussed.
>
> #define VEHICLE_OBD_BROADCAST_MODULE_TX 0x7df
> #define VEHICLE_OBD_BROADCAST_MODULE_RX 0x0
>
> These are used in the following call to get at the VIN.
>
> std::string response;
> int res = PollSingleRequest( m_can1,
> VEHICLE_OBD_BROADCAST_MODULE_TX, VEHICLE_OBD_BROADCAST_MODULE_RX,
> VEHICLE_POLL_TYPE_OBDIIVEHICLE, 2, response, 1000);
>
> questions:
>
> * Do you want one/both these in vehicle.h (I can always just have 0
> in place of *_RX in the call)?
> * If so, should I:
> o make a separate P/R or
> o just include it as a commit in the Ioniq-5 P/R ?
>
> //.ichael
>
>
>
>
>
> On Sat, 29 Oct 2022 at 22:48, Michael Balzer <dexter at expeedo.de> wrote:
>
> 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
>
> _______________________________________________
> 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/20221102/d11cebfb/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/20221102/d11cebfb/attachment-0001.sig>
More information about the OvmsDev
mailing list