Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CANParser: update_strings returns latest state when passed empty list #913

Open
fredyshox opened this issue Aug 8, 2023 · 2 comments · May be fixed by #1046
Open

CANParser: update_strings returns latest state when passed empty list #913

fredyshox opened this issue Aug 8, 2023 · 2 comments · May be fixed by #1046
Labels

Comments

@fredyshox
Copy link
Contributor

CANParser::update_strings returns most recently updated addresses, when called with an empty list.

Is it expected behavior? Shouldn't it return empty list of addresses, since nothing was updated?

Snippet showcasing this behavior:

parser: CANParser = ...

addr = parser.update_strings(cans)
# addr = {...}
addr = parser.update_strings([])
assert len(addr) == 0
# ^ this assertion fails
@sshane
Copy link
Contributor

sshane commented Aug 8, 2023

No it shouldn't return anything. Some confusing interaction with the timing variables in parser.cc is going on. We can choose not to update the vals in query_latest if the last_ts is 0, but unsure if anything relies on that behavior since last_ts not being 0 is an explicit condition of skipping updating the values:

opendbc/can/parser.cc

Lines 320 to 322 in 7d61776

if (last_ts != 0 && state.last_seen_nanos < last_ts) {
continue;
}

Also, here we set current_sec to the first CAN message, which we should probably set to the latest?

current_sec = last_sec;

@sshane sshane added the bug label Aug 8, 2023
@deanlee
Copy link
Contributor

deanlee commented Sep 5, 2023

We can choose not to update the vals in query_latest if the last_ts is 0, but unsure if anything relies on that behavior since last_ts not being 0 is an explicit condition of skipping updating the values:

currently, CANParser::__init__ relies on calling update_strings([]) to initialize vl, vl_all,ts_nanos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants