-
Notifications
You must be signed in to change notification settings - Fork 18
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
Allow interrupt-driven MACRAW operation #60
Conversation
This commit adds methods to RawDevice that enable interrupt-driven operation. The enable_interrupt() method sets up SIMR so that socket-level (internal) interrupts on Socket 0 cause chip-level (external) interrupts (and as a convenience also sets S0_IR as required). The disable_interrupt() method reverses those changes. The clear_interrupt() method acknowledges all interrupts and is intended to be called from the interrupt handler (or from thread mode soon afterwards). There is no change to existing functionality or operation if enable_interrupt() is never called. I did see PR#34 before filing this, but that change is focused on TCP and UDP sockets, and my use case is MACRAW mode. Tested on a W5500-Pico-EVB board with the RP2040 successfully receiving and acting on active-low GPIO interrupts from W5500 via the INTn signal on W5500 pin 36.
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.
Mind also adding a CHANGELOG entry for this? Thanks for the submission, this indeed looks quite useful!
src/raw_device.rs
Outdated
/// For instance, pass `Interrupt::Receive` to get interrupts | ||
/// on packet reception only. | ||
/// | ||
pub fn enable_interrupt(&mut self, which: u8) -> Result<(), SpiBus::Error> { |
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.
Would it make more sense to provide an enum of the interrupts that can be enabled instead?
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.
Well, the enum exists, it's register::socketn::Interrupt, but (presumably because in Rust you can't do enum|enum and have the result be of type enum), other parts of the code (notably Socket::set_interrupt_mask) take the u8 form instead. Probably the right answer here would be to use the bitflags
crate, but that's a wider change.
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.
Ah yeah indeed, good point thanks. But in an actual driver, you could use Interrupt::Receive as u8
so there would be no magic number. I think we're fine here as-is.
Also looks like |
Fixed, though the complaints weren't actually in the new code :) |
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.
LGTM, thanks! :)
This commit adds methods to RawDevice that enable interrupt-driven operation. The enable_interrupt() method sets up SIMR so that socket-level (internal) interrupts on Socket 0 cause chip-level (external) interrupts (and as a convenience also sets S0_IR as required). The disable_interrupt() method reverses those changes. The clear_interrupt() method acknowledges all interrupts and is intended to be called from the interrupt handler (or from thread mode soon afterwards).
There is no change to existing functionality or operation if enable_interrupt() is never called.
I did see PR#34 before filing this, but that change is focused on TCP and UDP sockets, and my use case is MACRAW mode.
Tested on a W5500-Pico-EVB board with the RP2040 successfully receiving and acting on active-low GPIO interrupts from W5500 via the INTn signal on W5500 pin 36.