-
Notifications
You must be signed in to change notification settings - Fork 22
Use VSS as mapping format for dbcfeeder #52
Use VSS as mapping format for dbcfeeder #52
Conversation
118ae5b
to
e978f7e
Compare
cdf39b0
to
e5a2729
Compare
9839dd2
to
205d0e9
Compare
dbc2val/dbcfeeder.py
Outdated
log.warning(f"Value ignored for dbc {vss_observation.dbc_name} to VSS {vss_observation.vss_name}," | ||
f" from raw value {value} of type {type(value)}") | ||
elif not vss_mapping.change_condition_fulfilled(value): | ||
log.info(f"Value condition not fulfilled for VSS {vss_observation.vss_name}, value {value}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have not tested it yet, but might the log level be too high? In the past we observed if just printing some message xxx times per second will slow down the feeder a lot/generate a lot of load. And just from looking at the code, this is not an error condition, but something that we might expect to encounter regularly. So maybe verbose?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be reduced to debug.
For now if running on info-level you get typically one line every time a signal is sent to broker (and if here is value is discarded), which could be quite many. So the question is - how verbose do we want to be on info-level, do we expect info-level to be usable for "real deployments" where performance is important?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking info should be usable for "real deployments", and might only be more verbose during initialisation etc. if this is not how things currently are, I can also live with letting "info" chat about every processed message, but then at least we should make sure the default log level when starting from command line or from container is set to warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of today default is INFO for dbcfeeder. I have no problems using DEBUG on recurring messages. But then , if you would be interested in knowing if any signals (at all) has been sent to databroker then you would need to change to DEBUG, and then you would get a lot of other messages as well.
new_val = item["to"] | ||
vss_value = new_val | ||
break | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be more clean/future proof to really check there is a math transform? And in else print something like "there seem to be transforms I just don't understand them"
I am thinking that might be more robust, if we add further transform types in the future and overlays/config files/dbcfeeder implementations get mixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that extract_verify_transform
shall do the static check that mappings are understood, so here only well defined mappings shall exist. But we could add an else-path here anyway. As long as we do not have too smart linters (that complains that the code is dead) it shall be ok.
dbc2val/mapping.md
Outdated
then you can annotate that file and use the annotated file in both KUKSA.val and the feeder. | ||
|
||
Annotating an existing VSS JSON file has however some drawbacks. If the JSON file is regenerated | ||
to support a new VSS version then the annotations must be manually transferred to the new vSS JSON. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type vSS->VSS
The `on_change: True` argument specifies that the VSS signal only shall be sent if the DBC value has changed. | ||
If none of them are specified it corresponds to `interval_ms: 1000, on_change: False`. | ||
If only `on_change: True` is specified it corresponds to `interval_ms: 0, on_change: True` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if on-change is true and an interval is given (not sure that is alway useful), how is the logic? I.e.
I open the door-> set open
Then the signal toggles millsions of times within one second, (interval_ms: 1000), so I assume it is not forwarded?
Then after the 1000ms IF the signal is coming again, is "on-change" based on the last reported or on the last observed value? Observed might be a problem because door->open=reported to VSS, door->closed not bcasue within timeout, and then will never be reported, if compare to last observed state.
Might be a corner case, but at least we should now - and preferably describe - what happens here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try to describe it a bit further down, will update it to make it clearer. The filtering is done in two phases. This partially depends on that we have a queue in between. I assume math-transform can be costly from performance perspective, and that is why we only do it time condition is fulfilled.
On_Change is always compared with last value sent to KUKSA.val. So theoretically, f you are very quick in opening/closing your door and you have an interval of 1000 ms then there is a risk that the opening not will be reported to KUKSA.val as we only "poll/sample" values every 1000 ms.
- If there is a time condition the time of the observation is compared with the stored value.
If the time difference is bigger than the explicitly or implictly defined interval
the stored time for the VSS-DBC combination is updated
and evaluation continue with the next step. - The DBC value is then transformed to VSS value. If transformation fails the signal is discarded.
- After transformation, if there is a change condition, the stored value is compared with the
new value. If they are equal the signal is discarded.
dbc2val/mapping.md
Outdated
|
||
Transformation may fail. Typical reasons may include that the DBC value is not numerical, | ||
or that the transformation fails on certain values like division by zero. | ||
If transformation fails the signal will be discarded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discarded->ignored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: I see the discarded
wording is just often/consistently. Will not mark every occurrence. can live with it :) I just would have written ignored
"$line$": 117, | ||
"mapping": [ | ||
{ | ||
"$file_name$": "test.vspec", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the
Although they are not in the "real" .json below if I see correctly?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it has been fixed in COVESA/vss-tools#211. So if using an older vss-tools you get them. I have not bothered to update the included JSON files here, it should not matter.
205d0e9
to
88eb510
Compare
PR updated. Based on our discussions on printouts I created a short status printout, also showing max queue size so far. this would give at least some indication if your feeder works as expected
|
Is this expected:
|
Does not work on Python 3.8, needs 3.10 (did not test 3.9). Is this documented? |
Default loglevel seems to be Info when run locally but warning in container. Should probably be the same? Info for both, now that info is less noisy? |
Our only statement on Python version is "Check that at least python version 3 is installed". In CI we use whatever Python-version is default on ubuntu-latest. Docker file use Python 3.9 so that 3.9 has been tested. I assume we likely can get it to run on Python 3.8, but we should better document what versions we intend to support. Python 3.11 is out - if we are to test all it will be a bit cumbersome to test all Python versions. |
I think for this specifically it is fine if we state 3.9 as minimum (I am not sure we would loose some typing features going back?). I think with the current state of feeders, what we should test is, what we also put in the container. Just document, so people know what to expect Was just surprised, as for the viss-client we made the choice 3.8 should be enough (there I think it makes sense) |
There are some minor changes between Python 3.8 and 3.10, I sometimes experience them for vss-tools, where CI use Python 3.8 (and I use 3.10 when running locally). This stackoverflow describe the root cause also this time, it was easy to adapt to 3.8, but I will add something on supported versions. |
88eb510
to
5e7054d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me
🐘
This is an implementation of #41
Apart from changing the config there are also some general refactoring:
dbcfeederlib
folder. This is to simplify inclusion in unit testingTesting fully performed:
Testing partially performed:
createvcan.sh
script