Skip to content

Generate raw signal getters and setters - #193

Open
felixvanoost wants to merge 4 commits into
oxibus:mainfrom
felixvanoost:generate-raw-accessors
Open

Generate raw signal getters and setters#193
felixvanoost wants to merge 4 commits into
oxibus:mainfrom
felixvanoost:generate-raw-accessors

Conversation

@felixvanoost

@felixvanoost felixvanoost commented Aug 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Generates new <signal>_raw_val getters and setters for every signal. These directly extract/pack the raw message payload bytes without any scaling or range checking.
  • Renames the existing <signal>_raw getters to <signal>_phys_val, representing the physical (scaled) signal value.

This fixes the previously confusing function name but is a breaking API change.

@felixvanoost
felixvanoost requested review from nyurik and trnila August 24, 2026 17:40
@felixvanoost felixvanoost self-assigned this Aug 24, 2026
@felixvanoost felixvanoost added the enhancement New feature or request label Aug 24, 2026
@nyurik

nyurik commented Aug 24, 2026

Copy link
Copy Markdown
Member

wouldn't this be really hard to update for the users? E.g. if they are using _raw now, would the meaning of that change?

@felixvanoost

felixvanoost commented Aug 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

wouldn't this be really hard to update for the users? E.g. if they are using _raw now, would the meaning of that change?

Users currently calling _raw() can simply rename to _phys() to achieve the same behaviour as before. It should be a simple change as long as users are aware of it. Perhaps a good opportunity to bump the release version?

Though perhaps slightly painful, I think this resolves a big inconsistency in the function naming.

@nyurik

nyurik commented Aug 24, 2026

Copy link
Copy Markdown
Member

right, but my point is that if we can make it backwards compat, perhas we should try to? A modification A->B + new A with the same signature is usually a path for foot-gun

@felixvanoost

felixvanoost commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

I'm happy to avoid the renaming if you prefer. We can keep the existing _raw() naming for the physical values, and I can call these new getter/setters _bits() instead. This is fully backwards compatible but IMO it now creates two naming changes that should be resolved at some point.

@nyurik

nyurik commented Aug 24, 2026

Copy link
Copy Markdown
Member

TBH I don't know what's better - I am not as much of a domain expert as others here, rather I focus on Rust itself. I am totally OK to rename things, as long as the old code fails to compile - thus helping users update it. If the meaning changes, but fn signature stays the same, we get a lot of non-obvious errors.

@trnila

trnila commented Aug 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

This could be safe and not surprising anyone while moving towards correct naming?

  • rename {signal}_raw to {signal}_phys / {signal}_phys_val to get scaled physical values (now with correct name)
    • current users will have to trivially rename function in order to compile
  • new function {signal}_raw_bits() / {signal_raw_val()} to get received bits/value without any scaling

Suffix _val is little bit longer, but it could be consistent at least.

@felixvanoost

felixvanoost commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

This could be safe and not surprising anyone while moving towards correct naming?

* rename `{signal}_raw` to `{signal}_phys` / `{signal}_phys_val` to get scaled physical values (now with correct name)
  
  * current users will have to trivially rename function in order to compile

* new function `{signal}_raw_bits()` / `{signal_raw_val()`} to get received bits/value without any scaling

Suffix _val is little bit longer, but it could be consistent at least.

I'm happy to rename to {signal}_phys_val and {signal}_raw_val for consistency everywhere. Any preference on phys_val/raw_val vs. val_raw/val_phys?

@felixvanoost
felixvanoost force-pushed the generate-raw-accessors branch from 4cff4d3 to 4a14b82 Compare August 25, 2026 19:42
@trnila

trnila commented Aug 26, 2026

Copy link
Copy Markdown
Member

phys_val / raw_val sounds more naturally - at least to me.

@felixvanoost
felixvanoost enabled auto-merge (squash) August 27, 2026 18:11
@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.91667% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/lib.rs 97.91% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@felixvanoost

felixvanoost commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

@trnila I've updated the function names, let me know what you think.

@nyurik any chance we could release v0.4.0 (#122) after this is merged since the API has changed?

@nyurik

nyurik commented Aug 28, 2026

Copy link
Copy Markdown
Member

sure, releasing is simple :)

@nyurik

nyurik commented Aug 28, 2026

Copy link
Copy Markdown
Member

note though that we can keep 0.3.x going if we also keep the old one as deprecated

@felixvanoost

felixvanoost commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Either version is OK with me, though there have been many feature additions and general updates since v0.3.0.

@nyurik

nyurik commented Aug 28, 2026

Copy link
Copy Markdown
Member

0.3.* is simpler to update to - i.e. it has no breaking changes (deprecations are ok as they are warnings) - so likely more ppl will update to it faster. If we can, we should stick to it

trnila
trnila reviewed Aug 28, 2026
View reviewed changes
Comment thread src/lib.rs
msg: &Message,
) -> Result<()> {
writeln!(w, "/// Get raw value of '{}'", signal.name)?;
writeln!(w, "/// Get physical value of '{}'", signal.name)?;

@trnila trnila Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we factor rendering comment into a function so its not duplicated with the code above?

Comment thread tests-snapshots/canpy/DBC_template.snap.rs
/// - Receivers: Node1
#[inline(always)]
pub fn value1(&self) -> CanMultiplexedValue1 {
let signal = self.raw.view_bits::<Lsb0>()[8..16].load_le::<u8>();

@trnila trnila Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we call the newly introduced function value1_raw_val?

Comment thread tests-snapshots/canpy/DBC_template.snap.rs
1 => CanMultiplexedValue1::One,
0 => CanMultiplexedValue1::Zero,
_ => CanMultiplexedValue1::_Other(self.value1_raw()),
_ => CanMultiplexedValue1::_Other(self.value1_phys_val()),

@trnila trnila Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The signal is raw value, but then we are passing physical value into _Other.
Not sure if physical values exists for multiplexor - isnt it just raw value (enumerator value)?

Comment thread tests-snapshots/canpy/DBC_template.snap.rs
#[inline(always)]
pub fn value1_raw(&self) -> u8 {
pub fn value1_phys_val(&self) -> u8 {
let signal = self.raw.view_bits::<Lsb0>()[8..16].load_le::<u8>();

@trnila trnila Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we call the newly introduced function value1_raw_val?

Comment thread tests-snapshots/canpy/DBC_template.snap.rs
/// - Offset: 0
/// - Byte order: LittleEndian
/// - Value type: Signed
/// - Unit: ""

@trnila trnila Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe we could omit empty units? and quotes?

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

Reviewers

@trnila trnila trnila left review comments
@nyurik nyurik Awaiting requested review from nyurik

At least 1 approving review is required to merge this pull request.

Assignees

@felixvanoost felixvanoost

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@felixvanoost @nyurik @trnila