[Ovmsdev] Ioniq 5 - Pushing up some prerequisite changes to components/vehicles

[Michael Balzer]

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.


>
> 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.


>
> 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.

>
> 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*).

>
> 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 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.

>
> 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).


>
> Thoughts? Shall I just push the code up to my own repo? Merge request?
>
> //.ichael
>

Regards,
Michael

-- 
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

-------------- 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/20221021/100b1fed/attachment.sig>