Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
UsartSpi
Support #562base: main
Are you sure you want to change the base?
UsartSpi
Support #562Changes from all commits
4a110c9
8ac3a86
2cd0cbd
82ad09a
ab5fad0
5debde9
a37cbc2
c5b076f
9ddb92b
b88cf04
d9bfd46
597b073
54990d0
46f4183
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Don't comment what code is doing, that should be obvious from reading the line below. Please comment why it is doing this — what's the purpose of setting UBBRn to 0 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.
Unsure of the purpose.
But code examples in the data sheet all do so, explicitly setting it to 0, before setting the baudrate (which I will update the calculation for)
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.
Don't enable the interrupt by default - it should always be explicitly enabled by the user through a method call, see e.g.
avr-hal/avr-hal-generic/src/usart.rs
Lines 300 to 308 in 65b304e
You don't need to implement this here, though — it should be part of the
spi
module.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.
UBRR sets the SPI clock frequency, so it should look something like this? I tested and it works, but I'm not sure if this is the best way to write the register in Rust.
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 you are certain on the numbers, you can make use of the commit 65b304e ("generic: spi: Assign discriminant values to clock rate enum") that I just pushed:
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.
Similar to the USART code, you also need to disable the USART peripheral here:
avr-hal/avr-hal-generic/src/usart.rs
Lines 506 to 510 in 65b304e
(The UCSRnB reset is the important part).
With that included, you can drop your comment.
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.
Hm, this is quite a hack. We really don't need the
cs
dance for MSPIM so it would be nice to somehow wrangle the SPI module to not require a CS pin for MSPIM...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.
This was done because I impliment SpiOps to cheap out on work, and not rewrite everything.
I can either reimplement a new spiops, or, force the use of a DummyPin internally (so users don’t have to worry about it) via the DummyPin crate (I believe that’s its name)
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 would be fine with defining a
UsartSpiDummyCs
pin type that needs to be passed toSpi::new()
to sidestep this. Pulling in an external crate for this is not a good idea, I think.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.
An alternative to the Dummy CS Pin (and what I believe my original thought process was), is how it is, where it's dynamic, meaning the CS can be handled by the driver in some which way, by manual toggle instead of SPI handling it.