Skip to content

Conversation

@rafaeling
Copy link
Contributor

No description provided.

@erikbosch
Copy link
Contributor

erikbosch commented Nov 7, 2024

What happens if you use an "old" config file, like the ones in https://github.com/boschglobal/kuksa-perf/tree/fix/cycle_time_microseconds/data

Will you get an error or will it silently just ignore the old setting? I would prefer that it gives an error so that it just not silently ignore the old setting.

Are there more existing scripts that needs to be updated?

@rafaeling
Copy link
Contributor Author

What happens if you use an "old" config file, like the ones in https://github.com/boschglobal/kuksa-perf/tree/fix/cycle_time_microseconds/data

Will you get an error or will it silently just ignore the old setting? I would prefer that it gives an error so that it just not silently ignore the old setting.

Are there more existing scripts that needs to be updated?

Forgot to push the modified files

@rafaeling rafaeling requested review from erikbosch and wba2hi November 7, 2024 11:09
{
"group_name": "Frame 2",
"cycle_time_ms": 20,
"cycle_time_microseconds": 20,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to make sure that it is intentional to change semantic meaning of the example, i.e. that we in some cases (like here) keep the numeric value, in other multiply be 100

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was actually non intentional, just left some value, provided more realistic values

Copy link
Contributor

@erikbosch erikbosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@argerus argerus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes a lot more sense to keep the "cycle_time_ms" in order to not break all old configs. In addition, I think it's a lot more common to specify cycle time in milliseconds rather than microseconds.

TL;DR
Add "cycle_time_us" in addition to "cycle_time_ms" instead of replacing it.

EDIT: Or skip adding "cycle_time_us" altogether and instead have the existing config accept a decimal number for the cycle time, e.g. "cycle_time_ms": 0.1.

#[derive(Deserialize, Clone)]
pub struct Group {
pub group_name: String,
pub cycle_time_ms: u16,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hardcoding it to either ms or microseconds (and choosing between a u16 and u32), it would be better to store it as a Duration. That's what we want in the end anyway, and would make it equally easy to set with either milliseconds, microseconds or anything else as input.

@argerus
Copy link
Contributor

argerus commented Nov 7, 2024

What happens if you use an "old" config file, like the ones in https://github.com/boschglobal/kuksa-perf/tree/fix/cycle_time_microseconds/data

Will you get an error or will it silently just ignore the old setting? I would prefer that it gives an error so that it just not silently ignore the old setting.

Are there more existing scripts that needs to be updated?

If we need to support specifying the cycle time in microseconds I would much prefer to add this in addition to the existing configuration option, i.e. support both "cycle_time_ms" and "cycle_time_microseconds". That has the added benefit of not breaking any existing test setups. It also removes the need to add a whole lot of zeroes when specifying common cycle times.

EDIT: Or just have "cycle_time_ms" accept decimal numbers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants