-
Notifications
You must be signed in to change notification settings - Fork 67
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
Improve race free register access ergonomics #127
Comments
So if I understand you correctly, this "coarse-grained" usage of your suggested API would look something like this: let mut gpioa = dp.GPIOA.split(...);
gpioa.pa1.into_push_pull_output(&mut gpioa.regs); Whereas a "fine-grained" usage would involve the user defining an The coarse-grained case looks good to me, much better than having to pass more than one register. The fine-grained scenario is much worse IMO. I don't see the benefit of having all that complexity. As you say, during the initial configuration the coarse-grained approach can be used without problems. The coarse-grained approach doesn't work well in the scenario where you want to keep two pins in different parts of your system and still be able to re-configure them. The only thing you can do then is putting the In my mind, the best solution is to just bite the bullet and use critical sections. This is certainly the best case for ergonomics: No need for the This approach has the drawback of disabling interrupts, but I'm not convinced that is more than a theoretical issue. The critical sections are very short (two register modifies usually) and are only invoked during the initial setup anyway. If someone can present a real-world use-case where such critical sections are actually an issue, we can still provide the "coarse-grained" way as an alternative, but I'd keep the CS approach even then because of the superior ergonomics. Btw, in case you haven't seen it, #37 has some good discussion around this topic too. |
Yes I have seen #37 and should probably have added a comment there instead. You are probably right about disabling interrupts, but as there at least for what I understand exists another way when using RTIC where the blocking would instead only block threads/interrupts of equal lower priority than the one with highest priority which is actually sharing the registers. So in that case one could actually keep the highest priority tasks/interrupts unaffected. And as you say there will probably be very few situations where splitting the registers would be of any real benefit, so it might not be worth it even if it might be a benefit in some rare scenarios. |
Let's move the discussion over to #37 :) But I prefer your issue title, so I'll use it for that issue 😉 |
I have played around and trying to figure out if there could be any ergonomic way to ensure race free access without any downsides, I do not think there is any perfect solution, but I think we could come a long way by replacing the current model where we pass one or more &mut parameters to the operations. We could instead implement a type state model where the user could join and split registers and set of registers, with one trait created for accessing each register. This should ensure that max one &mut parameter would be needed for each operation.
E.g. for the gpio the Parts struct returned by the GpioExt::split() operation would return a container struct containing all the gpio registers. Then it would be possible for the user to call split() on that struct to get individual registers ready to be merged again into more or less fine grained containers. I suppose the container structs should be created by the user based on need to not create type state explosion.
With this model it is then up to the user to select how find grained split they need and how they would like to ensure race free access. If the user selects a very coarse grained split, the APIs should be more ergonomic than today.
I also suppose that in most cases the user would not need to do any register splitting as the configuration is done only once start of the system.
The text was updated successfully, but these errors were encountered: