-
Notifications
You must be signed in to change notification settings - Fork 752
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
LPC: embassy-lpc55 hal base with gpio and pint driver #3343
Conversation
caadec6
to
25df26d
Compare
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.
Great to see new HALs! Embassy tries to make HALs that cover as widely as possible within a chip family. Is there any particular reason this is limited to lpc55 family? Would it make sense to make this into an embassy-nxp HAL instead? For comparison, the embassy-stm32 HAL covers a lot of chip families in the HAL, so it is certainly possible.
The simple answer as to why "lpc55" is because this is the only chip family I have at my disposal right now. Of course, I am not married to the idea of having a very specific hal for the lpc55. I would personally favour having an |
Apologies, I only read the code, not your entire comment 😅
I think it should go as widely as possible, at least if if it's to be put inside the embassy repo, because we don't want to end up with many hals for very similar chip families. IMHO the distinction should be on 'peripheral similarity'. If a UART across all NXP MCUs share 90% of the code, they should be in the same HAL.
You can read more about the reason for sealed traits here https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust/
It is the 'job' of the HAL to build safe abstractions on top of the PACs. I.e. ensure that registers are written in the correct order, and only by 1 'thread' (the borrow checker will help you with that). There's always the case that someone writes unsafe code and accesses the registers directly, but that's outside the scope of the HAL.
Often with a HAL, you need to figure out the "sweet spot" of how much flexibility you give in the API vs. how common the use case is. Maybe a limit of 8 is OK, it can be reworked later if someone has the use case.
You should be able to write comments in linker scripts using '/* */', just stating the attribution. |
Thinking about it, it should be safer to start off broad ( |
For what it's worth, NXP has gone through multiple acquisitions, and has a couple of very different chip families active at the moment: https://www.nxp.com/products/processors-and-microcontrollers/arm-microcontrollers/general-purpose-mcus:GENERAL-PURPOSE-MCUS The biggest three are the Kinetis (formerly freescale), i.mxrt, and LPC. I would be somewhat surprised if there was a meaningful amount of IP to share across these families, but I also do not use any of them actively (I've used all three in passing in the past, I don't remember them being particularly similar). embassy-nxp might work, but embassy-lpc, embassy-imxrt, and embassy-kinetis might be better splits. |
Searching around google a bit and speaking with a coworker who worked with more NXP boards, it should be possible to have a single hal. There is a C hal for the zephyr project that groups all of NXP chips in one (https://github.com/zephyrproject-rtos/hal_nxp), and I understood that the Kinetis and i.mxrt ones should be similar enough. LPCs are bit newer and do break away those two, but not completely. |
59bfd8b
to
cb84118
Compare
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 think this is probably a good starting point for an nxp hal, thanks!
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.
@george-cosma It just occured to me, could you add the examples to CI as well?
Sure thing |
cb84118
to
c1933e5
Compare
bender run |
2 similar comments
bender run |
bender run |
|
c1933e5
to
e7e245e
Compare
@lulf Fixed it with a dummy force-push 😄 |
Overview
This pull request implements a simple embassy-lpc55 hal based on
lpc55-pac
. This PR only implements the GPIO driver and the PINT (pin interrupt) driver, which is, in turn, used internally to detect edge & level changes for input GPIO.TODO / Help Wanted
While there are many drivers that are yet to be implemented (most notably a time driver, which I want to tackle soon!), I'm going to limit this PR to GPIO and PINT. Despite the limited scope, there are some design decisions of which I am unsure about.
First, how should the hal be organized in terms of chip-specific details? I thought about making this hal similar to how
embassy-nrf
does it (each nrf family has a.rs
file that creates the unique configuration, but the drivers are more or less identical). However, I am not that familiar with the other chips in thelpc
series. I would personally leave it as-is, and deal with this when the problem of another chip arises, but I thought I should get your feedback on this first.Note: although I named the hal
embassy-lpc55
, it only functions for a subset of these: the LPC55S6X, LPC55S2x and LPC552X series. There are other series as shown here. However, of these I have tested the hal only on the LPC55S69, as I have the LPCXpresso55S69 board or sometimes LPC55S69-EVK.Second, what are the differences between
Pin
andSealedPin
inside of the GPIO driver? When creating this hal, I took inspiration from theembassy-nrf
hal and theembassy-rp
one. However, I'm unsure of the distinction between these two? My guess would be thatSealedPin
has something to do with the fact that it doesn't need to be implemented on a Peripheral? I'm unsure. If I can understand what the deal with it is, I can document the driver properly, and perhaps cut down on the amount of abstraction.Thirdly, I noticed in the other hals that pac access is usually done by getting a raw pointer to the relevant register block. This seems fine to me, I've integrated this idea in the
pac_utils.rs
file, but I'm unsure if it is actually safe. If the underlying pac handles read/writes atomically, then this isn't an issue. If the opposite is true, registers could end up in an unexpected state if modified from different threads - a race condition. That being said, the current code would not be affected by this due to the fact that each PIO is owned exactly once. The only time the.modify()
action is taken on a register, is when the registers are specific to each pin. As such (unless a circumvented by unsafe rust), only one thread could ever modify a pin-specific register. However, putting pac access behind anArc<RefCell<...>>
wouldn't be that difficult either.And a final issue I'd like your feedback on - how should we detect level changes? In this PR I used the Pin Interrupts, but these are designed so they trigger an interrupt only for a specific, user-configured, pin. In total there are only 8 pin interrupts, so if a user wants to wait on more than 8 input changes at a time, they can't.
The LPC55S6X/LPC55S2x/LPC552X user manual also mentions a "Group Interrupt" peripheral (GINT). This allows for multiple pins to contribute to an interrupt, however after the interrupt triggers there is no way to know which pin was the cause. A software implementation that determines the differences between pre-interrupt and post-interrupt would be needed.
So yhea, that's about it. It works, but with a lot of unanswered questions :D - any feedback on this would be appreciated!
Acknowledgements
The
memory.x
file was based onlpc55-hal
's but I'm unsure how to insert that attribution inside of the.x
file itself.