Start on changes to GPIO HIL interface. #112
Conversation
|
We pulled the notes from today's conversation into https://github.com/helena-project/tock/projects/2 The goal would be to finalize this interface on next week's call and start talking about the timer interface |
alevy
added
the
blocked
|
I'm adding the "blocked" label just a marker that this PR isn't for reviewing/merging yet. |
alevy
force-pushed
the
hil-gpio
branch
2 times, most recently
from
2b7f42f
to
38d5fbb
Compare
alevy
removed
the
blocked
|
Ready for some reviewing |
| fn disable_interrupt(&self); | ||
| } | ||
|
|
||
| /// Interface for users of synchronous GPIO. In order | ||
| /// to receive interrupts, the user must implement | ||
| /// this `Client` interface. | ||
| pub trait Client { |
There was a problem hiding this comment.
Not GPIO-specific, but why is set_client() not part of the HIL? The Client trait is part of the hil.
There was a problem hiding this comment.
This is a result of the discussion we had on the phone this Monday. I'm still split on it still, though.
The high bit is that it adding it requires also adding a lifetime parameter to the HIL trait, which makes the types for each implementer a bit gnarlier.
@phil-levis's thought we should prefer to make it as accessible as possible for people developing capsules and ports for chips.
There was a problem hiding this comment.
Right. I thought there was a reason for it. I do agree with the decision from Monday.
There was a problem hiding this comment.
The more I play around with the time hils, and in particular the AlarmToTimer type, the more I disagree with this decision.
In the case of GPIO, it means that if you write a driver that uses a GPIO pin for interrupts (e.g., the TMP006), you have no way of ensuring that you're indeed set up to be the client of that pin. Instead, you have to rely on the platform integrator to remember to call set_client, in some chip-implementation specific way, at some point after initializing both your driver and the pin.
There was a problem hiding this comment.
This is a good point. Combined with Brad's point below about a single driver having multiple possible client interfaces depending on it's mode I'd say we should switch to set_client in the HIL interface.
It's annoying to have to carry the silly Rust lifetime syntax around everywhere, but it's not annoying enough to be willing to limit functionality.
|
I may be a little out of the loop, but regarding Something like: impl USART {
...
}
impl hil::spi::SpiMaster for USART {
fn set_client(self, hil::spi::SpiMasterCallback) { }
}
impl hil::uart::Uart for USART {
fn set_client(self, hil::uart::Client) { }
} |
|
The argument is that actually the code would end up looking like this: impl USART {
...
}
impl hil::spi::SpiMaster<'static> for USART {
fn set_client(self, &'static hil::spi::SpiMasterCallback) { }
}
impl hil::uart::Uart<'static> for USART {
fn set_client(self, &'static hil::uart::Client) { }
}and the Console code would look something like: struct Console<U: Uart<'a>> {
uart: &'a U
...
} |
| @@ -144,7 +144,8 @@ impl<'a> TMP006<'a> { | |||
|
|
|||
| fn enable_interrupts(&self) { | |||
| // setup interrupts from the sensor | |||
| self.interrupt_pin.enable_input(InputMode::PullUp); | |||
| // TODO(alevy): do we need to make sure it's a pull up? | |||
There was a problem hiding this comment.
Yes, this line does need to be pulled up. The interrupt line form the TMP006 is open-drain and requires a pull-up.
There was a problem hiding this comment.
Do you think this is something that should be in the board config (that sets up the driver) or in the TMP006 driver? In principal I suppose the board might have a physical pull-up resistor...
There was a problem hiding this comment.
Oh yeah, I'm totally happy with it being in the board main.rs file. Most boards that use a TMP006 would have a pull-up and wouldn't need to do anything at all. It just so happens that Firestorm does not.
|
I've gotten rust to compile without it: https://github.com/helena-project/tock/blob/usart-spi/chips/sam4l/src/usart.rs#L489 which is what I based my example off of. |
Not much here except for comments. Highlights: - Changed `enable` to `make` when setting up GPIO pins. - Changed `GPIOPin` to just `Pin`. This better mirrors `Client` (which is not `GPIOClient`) and should be fine when using namespacing (i.e. `hil::gpio::Pin`). - Document what `disable` is supposed to do.
Separate out Pin and PinCtl Port boards, chips and capsules to new interface
Not much here except for comments. Highlights:
enabletomakewhen setting up GPIO pins.GPIOPinto justPin. This better mirrorsClient(which is notGPIOClient) and should be fine when using namespacing (i.e.hil::gpio::Pin).disableis supposed to do.