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

Fix gte errors #43

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Fix gte errors #43

merged 1 commit into from
Oct 24, 2023

Conversation

evading
Copy link
Contributor

@evading evading commented Oct 24, 2023

This change fixes some issues where watermark maximum sizes were interpreted the wrong way. Leading to watermarks clamping to the maximum value instead of disabling interrupts on overflow, as per the spec.

Thank you!

Thank you for your contribution.
Please make sure that your submission includes the following:

Must

  • The code compiles without errors or warnings.
  • All tests pass and in the best case you also added new tests.
  • cargo +stable fmt was run.
  • cargo +stable clippy yields no warnings.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You add a description of your work to this PR.
  • You added proper docs (in code, rustdoc and README.md) for your
    newly added features and code.

@evading evading requested a review from a team as a code owner October 24, 2023 06:19
@evading evading force-pushed the fix-gte-errors branch 2 times, most recently from 3b0e81e to 89e30fa Compare October 24, 2023 06:33
@vccaedholm
Copy link
Contributor

Looks good to me

Copy link
Contributor

@Ironedde Ironedde left a comment

Choose a reason for hiding this comment

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

Nice find, thanks for the addition! 😃

Some code seems to have been brought over from #41 which was just merged.
Please rebase and remove the duplicate code =)

Also, add a reference to this PR in the changelog.

mcan/CHANGELOG.md Outdated Show resolved Hide resolved
mcan/src/bus.rs Outdated Show resolved Hide resolved
mcan/src/bus.rs Outdated Show resolved Hide resolved
mcan/src/reg.rs Outdated Show resolved Hide resolved
mcan/CHANGELOG.md Outdated Show resolved Hide resolved
This change fixes some issues where watermark sizes were interpreted the
wrong way. Before this commit, watermarks for Rx FIFOs and Tx Event FIFO
were clamped to the maximum permissible watermark. But the behavior
according to the users manual is that watermark sizes over the
maximum permissible will disable the corresponding interrupt.

P.31 and p.34 of the M_CAN users manual v330 states that Rx FIFO
Watermarks, FnWM, can take values 1-64 inclusive. Any value greater than
64 is interpreted as "Watermark interrupt disabled".

P.44 of the M_CAN users manual v330 states that the Tx Event FIFO
Watermark, EFWM, can take values 1-32 inclusive, Any value greater than
32 is interpreted as "Watermark interrupt disabled".

This change makes the implementation consistent with the users manual on
those specifics.

A value greater than the maximum will be changed to zero for
consistency. The bit ranges are 6 bits wide for the Tx Event and
7 bits wide for the Rx configs but the u8 datatype is used in the
software interface. That u8 will be masked and shifted so that f.ex.
writing 129 to either bit range will actually set the value to 1, not
consistent with the user manual.
Clamping to any value over the maximum seems arbitrary, and for that
reason values larger than the maximum permissible are set to zero, which
also disables the interrupt.
Copy link
Collaborator

@epontan epontan left a comment

Choose a reason for hiding this comment

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

Nice find! LGTM 👍

Copy link
Contributor

@Ironedde Ironedde left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the issues.
Approved 👍

@Ironedde Ironedde merged commit 39c0726 into GrepitAB:master Oct 24, 2023
5 checks passed
Ironedde pushed a commit to Ironedde/mcan-clone that referenced this pull request Oct 24, 2023
- Added
 - Add `Can::aux::initialization_mode` (GrepitAB#41)

- Changed
 - Fix some issues with watermark sizes for Rx FIFOs and Tx Event FIFO (GrepitAB#43)
 - Adhere to `filter_map_bool_then` clippy lint (GrepitAB#42)
@Ironedde Ironedde mentioned this pull request Oct 24, 2023
Ironedde pushed a commit to Ironedde/mcan-clone that referenced this pull request Oct 24, 2023
- Added
 - Add `Can::aux::initialization_mode` (GrepitAB#41)

- Changed
 - Fix some issues with watermark sizes for Rx FIFOs and Tx Event FIFO (GrepitAB#43)
 - Adhere to `filter_map_bool_then` clippy lint (GrepitAB#42)
Ironedde added a commit that referenced this pull request Oct 24, 2023
### Added
 - Add `Can::aux::initialization_mode` (#41)

### Changed
- Fix some issues with watermark sizes for Rx FIFOs and Tx Event FIFO
(#43)
 - Adhere to `filter_map_bool_then` clippy lint (#42)
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