-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add gpio functionaliy to lpc845 #155
Conversation
Thanks for the pull request, @david-sawatzke. I haven't taken the time to look at it in detail so far, but I have a note about the general approach. I think the large number of For example, why define I haven't tried this myself though, so maybe there are problems I'm not seeing right now. In any case, I think it's fine to merge the current approach now and find better solutions later.
There's no policy right now, except that I decided not to use them years ago without ever revisiting that decision. If using ROM routines is required to get something to work, then that's what we're going to do. |
This approach will probably lead to code duplication, e.g. with syscon I'd add an unsafe We'd also have to add constructors to the structs describing parts of the |
With FRG & FRO equivalent but slightly different replacement are available
Not sure what I think. What you say makes sense, but I don't think I fully grasp all the issues right now. I need to get my hands dirty and see for myself what's possible and what's not. Only problem is to find the time to do it :-) |
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.
Finally got time to review this. Sorry for the delay.
I left some comments, but since all of them are nice-to-haves, I'm going to merge this immediately. My general concern about too much #[cfg(...)
in the common crate remains, but as long as I only have a vague feeling that it could be done better, that's no reason to delay any pull requests.
Thank you, @david-sawatzke!
#[cfg(feature = "845")] | ||
use crate::raw_compat::gpio::{CLR, DIRSET, PIN, SET}; | ||
#[cfg(feature = "82x")] | ||
use crate::raw_compat::gpio::{CLR0 as CLR, DIRSET0 as DIRSET, PIN0 as PIN, SET0 as SET}; |
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.
We can keep this as-is for now, but this looks like something that should happen in raw_compat
.
#[cfg(feature = "845")] | ||
use crate::raw::syscon::{ | ||
pdruncfg, presetctrl0 as presetctrl, starterp1, sysahbclkctrl0 as sysahbclkctrl, PDRUNCFG, | ||
PRESETCTRL0 as PRESETCTRL, STARTERP1, SYSAHBCLKCTRL0 as SYSAHBCLKCTRL, |
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.
Same. Looks like a job for raw_compat
.
loop { | ||
// For this simple demo accurate timing isn't required and this is the | ||
// simplest Method to delay | ||
for _ in 0..1000000 { |
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 be nice to add a comment here about which optimization level this was tested with. Compiling with or without --release
makes a big difference when using this method :-)
raw_compat
(Unfortunately couldn't find a way to do the same for registers)pin_state::Adc
topin_state::Analog
.Peripherals
to lpc845. As the gpio is per default disabled, this also meant adding an additionalnew
constructor(?) for disabled gpio peripherals and renaming the old one tonew_enabled
.As an aside: What's your policy about rom routines, as the fro (internal oscillator) of the lpc845 doesn't seem to be configurable otherwise