[Ovmsdev] Ioniq 5 - Pushing up some prerequisite changes to components/vehicles
Michael Geddes
frog at bunyip.wheelycreek.net
Wed Nov 2 17:04:14 HKT 2022
Excellent,
Pull request has gone up for Ioniq 5
https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/762
I hope I haven't pre-empted some of the config things/defaults.
There is still a bunch of work I'd like to do in discovering more
addresses... and I'm kinda hoping that somebody with better resources can
fill in some controls (I got as far as confirming that none of the ones
from the Kia/Kona work).
For eg I found accelerator pedal percent, but not the brake, and I've
confirmed all the door and lids.. except the charge port one!
Unfortunately I also don't have a second driver in the household (who can
physically drive my car without extra controls) so I can't drive around and
query stuff.. but I have a friend who might be up for it.
The VIN I get is a weird partial thing with the letters (in a weird order)
and no number. NFI if it's encoded weirdly or what.
I have an OBD splitter on the way that might help me work out why the HUD I
bought doesn't work (at the least I can test the voltage drop).
//.ichael
On Wed, 2 Nov 2022 at 14:57, Michael Balzer <dexter at expeedo.de> wrote:
> 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:
> - make a separate P/R or
> - 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 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
>>>>>>
>>>>>
>>> _______________________________________________
>>> 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
>>>
>>
>> _______________________________________________
>> 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
>>
>
> _______________________________________________
> 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/20221102/a78ffb4d/attachment-0001.htm>
More information about the OvmsDev
mailing list