search

COVESA / vss-tools

Software for working with VSS (https://github.com/COVESA/vehicle_signal_specification)
Mozilla Public License 2.0
49 stars 51 forks source link

add const UID attribute to allow constant IDs #352

Closed nwesem closed 2 months ago

nwesem commented 2 months ago

Hi this PR adds constUID attribute to allow overwriting generated static UIDs, as for static UID this is a 4-byte hex identifier. Please see test test_const_id for more details

erikbosch commented 2 months ago

Should usage possibly be described in https://github.com/COVESA/vss-tools/blob/master/docs/vspec2id.md

nwesem commented 2 months ago

Should usage possibly be described in https://github.com/COVESA/vss-tools/blob/master/docs/vspec2id.md

Thanks for the hint! I will add documentation as soon as we decided on how this will be integrated

erikbosch commented 2 months ago

MoM:

erikbosch commented 2 months ago

I assume there is still some work remaining, like documentation. Correct @nwesem? If possible it would be great if you could finish during the day, as this is the last PR intended to be included in 4.2, so that I could prepare a release candidate tomorrow.

nwesem commented 2 months ago

thx for letting me know @erikbosch, I'm currently working on the documentation. Will be done soon. Would it be possible to include my suggestion for this issue https://github.com/COVESA/vss-tools/issues/354 in the release candidate as well? I can do it for the shown min and max variables and send another PR by the end of the day. It's quite important to make static UID file iteration work.

nwesem commented 2 months ago

@erikbosch quick question: to fix the bug mentioned in https://github.com/COVESA/vss-tools/issues/354 I will branch off from here and merge back into this branch feat/const-id? Otherwise we would probably run into merge conflicts as I would be changing the same files.. do you agree that makes sense?

erikbosch commented 2 months ago

That works fine, no problem to use the same branch, or to use the "same" commit in multiple branches. Use what is easiest for you, as long as it is understandable what is the intended final result I have no problems fixing minor merge issues when a PR is to be merged.

nwesem commented 2 months ago

There is an issue. I implemented the default values for min and max and updated everything regarding the static UIDs but setting the default value causes 43 other tests to fail (AssertionErrors) because now min and max values are null or None instead of the empty string "".

@erikbosch Do you have a second to take a look and help me on this? I pushed it to branch https://github.com/nwesem/vss-tools/tree/fix/var-init The other option is to find a somewhat hacky solution to make static UIDs work for 4.2 release and then properly solve it for 4.3 release. I gave a hacky solution a quick try, I could make it work but yeah that's definitely not good practice :smiley: What do you think? It could also be worth spending the second to update the tests according to the new default values for min and max but I cannot estimate how much work that would be..

Also fyi @adobekan

nwesem commented 2 months ago

Hi @erikbosch I found a decent way to temporarily fix it here https://github.com/COVESA/vss-tools/pull/352/commits/ad0b093338f389dbb7bfb7697af0ceed50a526e9 which means this PR is ready for 4.2 release. You can disregard the comment above for the moment. However, I will look into the initialization issue asap, but that needs more time. Let me know if this works for you.. Thx!!

erikbosch commented 2 months ago

Thanks @nwesem - that works for me. I will take a look at the PR and let you know if I have any concerns.