[Ovmsdev] Ioniq 5 - Pushing up some prerequisite changes to components/vehicles
Michael Geddes
frog at bunyip.wheelycreek.net
Wed Nov 2 07:54:55 HKT 2022
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20221102/69821de1/attachment-0001.htm>
More information about the OvmsDev
mailing list