-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
GPIO: need generic API for optimized pin read/write operations #11917
Comments
I'm seeing dozens to nearly a hundred clock cycles for a plain GPIO set or clear on several platforms when using gpio_pin_write(): nrf51, nrf52, k64f, and l476rg. All of these should be able to be toggle an output with a couple instructions. |
Need for what? Setting/clearing bits in Zephyr works as follows:
It's unclear how to reconcile this view of Zephyr with a fact that specific SoCs has a way to toggle a limited subset of pins (builtin into SoC) using a couple of instructions. |
@pfalcon That's a fair point; I'm doing this in main() and probably shouldn't expect maximum speed in all cases. I've been comparing against direct use of the Nordic memory-mapped GPIO peripheral structures in the same code, which work and are much faster, so it's at least theoretically possible on some architectures in some contexts. I suspect @mbolivar's original use case of bit-banged protocols implemented in a driver, might be different. Or not. I'm not clear yet on where all the privilege levels get transitioned and how. |
@pabigot, so my point is that we need to keep that "wannabe a real protected OS" shift-face of Zephyr in mind. A usecase of "no kernel/userspace split" aka "kernel mode only" definitely exists, but I don't see organized stackholders for that mode unfortunately, whereas a need to support userspace mode and syscalls rampages thru Zephyr. And maintaining 2 sets of APIs requires an organized effort, otherwise we just spread too thin.
Yes, that's what I'm asking abot too. If the idea is that those are just internal helper macros to allow to implement kernel-level bitbanging drivers, that's fine, just need to define that clearly. But I don't see bitbanging drivers to be implemented too much in Zephyr (again, maybe the interest is just ramping up). |
Userspace and kernel separation, aka a) Propose a new set of macro or codegen based-APIs that work only when userspace is disabled Paging @andrewboie as the userspace maintainer |
I have no idea what you are talking about. We surely support supervisor-only use-cases as a first class citizen in Zephyr. Rampages? Really?
The first two steps are skipped if either CONFIG_USERSPACE is disabled, or the caller isn't running in user mode. There's no overhead to this feature if you don't use it.
I'm 100% ok with this option. |
Yep, we need to adjust many APIs for userspace constraints like no callbacks, excessive argument introspection, checks, and copying (just recently I had to remember why Linux developers don't like ol' good ioctl(), and I have no idea what concerns will be raised for implementation of it as a syscall in Zephyr), and non-obvious API implications like described in #10911. Skipping long boring talk like that, "rampages" is a good word to describe it, YMMV ;-).
Well, that's good to know! But how to organize it all and support so different configurations, e.g. how to even document them well? I'm glad to know there're people who want to work on that. And we aren't getting qemu_x86 back into nice simple cozy CONFIG_USERSPACE=n world, are we? |
We didn't change the driver subsystem APIs when we added system calls. We exposed a lot of the APIs to userspace, but they remain the same as they were before.
You think it's a good idea to allow user mode to register callbacks that run in interrupt context? You can still register them in supervisor mode.
Excessive? You are aware this is a security feature, right? What checks would you like to remove?
Don't expose them as system calls...this ticket becomes completely orthogonal to userspace.
Non-obvious to people who can't be bothered to read the documentation I suppose.
Huh? |
Hi there. I want fast GPIO bit-banging APIs behind a standard Zephyr interface in particular to implement efficient LED strip protocols. Some chips, such as the WS2812 (https://wp.josh.com/2014/05/13/ws2812-neopixels-are-not-so-finicky-once-you-get-to-know-them/), have one-wire protocols with tight timing requirements. I currently implement those in a generic way using SPI, but it's a hack (which has required additional hacks, such as bdde886). There's a bit-banging driver (drivers/led_strip/ws2812_sw.c) also, but it's nRF-only. I would like to be able to implement a driver in Zephyr using something similar to what the FastLED (http://fastled.io/) library implements in C++ in an Arduino context: code which compiles down to a few assembly instructions to hit bit set / reset registers directly and efficiently, which can be called in a loop with interrupts disabled to blit a line of lights. So no, this has nothing to do with userspace. |
Well, so I don't think it's a good approach in general. But otherwise, what I meant is probably "adjust design of APIs", first of all applied to new APIs. And per above, I don't even understand fully what design adjustment repertoire is needed in general, still learning (why I bring up such discussions).
Absolutely not. Actually, I rely on that as a litmus tests whether an API is general enough or, well, "kernel mode only". And actually, seeing that you're kinda flexible on these matters, I start to worry that something might be cooking, and one of these days you may submit PR "support callbacks into usermode". That would crumble down whatever criteria I've learned so far re: kernel vs userspace APIs. Because otherwise, everything is implementable. Linux somehow implements signal handlers ;-). And to clarify again: I really think that using "normal IPC" for "native Zephyr API" is better than trying to implement callbacks (which we may need one day anyway for deeper aspects of POSIX subsys).
None. I need to implement quite a lot of such checks for syscalls which I in turn yet need to implement. That quite complicates my developer's life ;-). Sorry for whining again ;-).
Nope, only that with a config like that, absolutely nobody dealing with Zephyr can escape the need to keep usermode in mind when designing APIs. If we find maintaining dichotomies of kernel-only vs userspace-capable APIs a suitable approach, fine. Worth to have been discussed IMHO. |
Since you're still learning, is antagonistic rhetoric like "rampage" really necessary when by your own admission you don't fully understand this feature or the design decisions that went into it?
Yes, we'd have to implement something like signals. Nothing cooking on this front at the moment. Mostly because the workaround (installing callbacks at app startup before dropping to user mode) doesn't give anyone any heartburn.
People should be free to ignore user mode when designing new APIs for Zephyr is what you are saying? I mean, they can, but such APIs may not be availble to user mode. I'm struggling to understand what the problem is here. Other than you don't like user mode because it isn't needed for your particular use-cases and would rather it simply wasn't there at all. btw, if you actually look at boards/x86/qemu_x86_defconfig user mode isn't turned on there. It is however turned on when CONFIG_TEST is enabled for all of our test cases. |
Sounds good, but expect questions like this: |
No, I'm saying the opposite - ideally, there should be a single API which works in both modes. Designing kernel-only API should be an exception with very well understood usecases. Otherwise we get maintenance, documentation, and community confusion issues. |
I hope @andrewboie will correct me if I am wrong, but I wouldn't expect the GPIO-specific APIs I commented about to be available to userspace -- can't have userspace touching device registers directly IIUC. My dream is that in-kernel drivers which use GPIOs can do so more efficiently while still using generic APIs if they need to. I believe I've given an example where this is needed. Are you opposed to the idea in general, or have some other concern(s)? |
I'm not sure if I explain myself not clear enough, especially given that @andrewboie criticizes me for using too strong words like "rampage" (though I don't see anything "antagonistic" in it, but ok). So, let me be clear - you wouldn't expect these GPIO APIs to be available to the userspace, but users will. So, please be prepared to document them in such a way as to make it very clear where they work and where they don't. |
@andrewboie, disclaimer: I'm not trying to be antagonistic. But you just made my hair turn gray, on the suspicion that I hallucinated things here #10972 (comment) , and well, large part of (gone offtopic definitely) content here. How, how a commit like 1c5642a could have been done? Let me be short and polite - please turn it back, or it never should have been turned on in the first place. But now we need to have it on, if we want all the stuff being discussed here to be paid attention by the people, or we'll have wildest regressions re: userspace functionality. |
CONFIG_TEST turns it on. This gets enabled by all of Zephyr's unit tests. There isn't a problem here. This includes the socket tests. The x86 emulator isn't the right place to enable it. For one thing, we have to test on ARM and ARC too, which also implement user mode. And we need to test on real hardware as well.
No, we won't. Have you looked at tests/Kconfig? |
"wannabe" was another cute one |
I agree with @carlescufi proposal. To precise a bit: I guess you mean that the macros are unusable in userspace and not when CONFIG_USERSPACE is set? Either way, kernel space will be able to use these new macros right? |
Just some food for thought: the first few versions of the Arduino programming environment provided functions such as What they did in the end was providing a macro-based version of |
I think that a 'one size fits all' solution may prove a challenging goal for any software project. For the record, after the conversation with @carlescufi @erwango and @galak at ELCE I scrambled the code that Zephyr generates when toggling a gpio on a nrf52 SoC with CONFIG_USERSPACE=n The call to gpio_pin_write() assembles to some 38 instructions. Compare this to the 3 instructions required to do the job. The question for me is: Is it worth to go thru more macro/code generation magic to solve it ? It seems it depends on the final use case. Just because I've been recently looking at cpp templates and as I see that @pfalcon was involved in a cpp peripheral template driver, I dare to mention here that IMO cpp shines in its capabality to use compile time abstractions that neatly solve the use case at hand. |
@xnlcasad +many. A lot of Zephyr's APIs appear to be designed around least common denominator, making them suitable for toy applications but not real-world products. The sensor API is one. I'm still formulating my thoughts on this, though, so don't want to start a digression. Also C++: Prior to discovering Zephyr I did a complete C++-17 framework for Nordic devices (NRF51 and NRF52) using nothing but the CMSIS headers. Templates, CRTP, Currently Zephyr forbids use of C++ outside of applications, even though there is no technical reason I can see why it can't be used in drivers as long as it avoids certain features (exception, dynamic memory allocation, etc.). The more time I spend with things like the GPIO and ADC infrastructures the more strongly I'm tempted to port over what I did for that project. |
I agree with the premise in general. Some APIs have been updated to try and cover the advanced functionality of the different underlying hardware variations, but there are many that still require work. Our weekly API meetings help, but we need more contributions in order to move at a swifter pace.
I've copied you on the relevant issue there.
The issue I see there is forcing C++ onto users. Depending on the toolchain and application this might be a burden, and this is also one of the reasons that the APIs need careful consideration. But this is the second time this suggestion has been posted now, so I think it deserves more attention. |
And that's too much! With bitbanding as supported by many (but not all) ARMs, that would be 1 instruction to set a pin to an exact value.
Thanks for bringing it up ;-). Nowadays, I guess one would argue for need to go the Rust way: https://github.com/rust-embedded/awesome-embedded-rust . All in all, I jumped on this thread because I'm an efficiency geek and very, very love this stuff. Tell me that something costs less cycles and less bytes - and I'm ready to vote it up no matter what it is. So, it was an act of self-policing and "raising awareness" that there are hidden costs with adding adhoc APIs, like maintenance, documentation, community confusion. And it's rather ironic that while arguing for possible confusion, I showed that I'm completely confused myself about another Zephyr feature - "userspace". So, it's not just one-off GPIO case. Surely, we need those macros. But how all those features, gathered together, make Zephyr a coherent, easy to use system is an open question. |
@lpereira I think an approach like that is probably workable in my use case, as long as the pin I'm bit-banging is a compile-time constant provided in a DTS overlay or so (which the driver implementation could ensure). |
Userspace/kernelspace expectancy confusion seems like a complete non-issue - just expose the "unsafe" API in a clearly named header. |
Moving a comment over from #15611....
@mnkp @pabigot to follow up on our brief discussion today, I thought I would look at all the existing GPIO drivers and see what types of registers they use. From there, I think we can see more clearly what this would look like in practice. Here are my results (plain text formatted). Comments/corrections welcome; I did this by inspection:
From this, I think it's clear that if we provide "write register" accessors for both bit set/clr registers and output registers, as well as one "read register" accessor, we can cover almost every interesting case. The write register APIs could look something like this:
On the output side, driver authors would need to try both gpio_get_output_register() and gpio_get_bit_set_register() / gpio_get_bit_clear_register() as appropriate for the use case. For quick hacks or applications that know which one their SoC supports, you could just call the right one. On the input side, a single gpio_get_input_register() seems to cover all the drivers where something like this is possible. |
Regarding the gpio, has any thought been given to configuration of the gpio? That is, you can typically configure an output as digital common drain, p or n open drain, analog or high impedance. Inputs can be configured as digital input or analog input. Inputs and outputs can have additional attributes such pull up and pull down. |
This issue is specifically about optimized read/write, so it's not the right place to ask this question. Please refer to #15611 instead. You can also see the new gpio.h API in the |
I will have a look at that issue, thank you. However, I disagree that this issue is not related. In fact, it Is desirable to have optimized changes of state from the various output or input capabilities of a GPIO. That is, the GPIO states embody more than “low” and “high” and this API should reflect that. |
The API operates on logical levels, low and high. You are talking about pin configuration; it's not to be discussed here. |
API 2020-07-07: Moved from triage to on-hold. Open for 18+ months, agreed useful, nobody has intent to work it. |
+1 to an optimised read/write/toggle interface. The Itsybitsy M4 Express has a DotStar LED on an SPI-like interface but it's not routed to a physical SPI port. I'd like to do a generic software only SPI driver but it needs write and toggle operations. |
Its connected to PB02/PB03, which can be connected to SERCOM5 pads 0 and 1. How is that not a physical SPI port? |
Hello everyone, However, this driver contains various nRF-specific functions on the main-branch (despite various PRs attached here). In the PR of @mbolivar the OnOff-manager was not used yet, but also there nRF specific code was used. |
@tobiasjaster Please open a new issue to discuss this |
I think we need macro-based APIs to avoid function call overhead to set and clear bits, which allow SoC-specific code to provide small and fast assembly implementations. This would be needed for time-sensitive bit banging protocol implementations.
Originally posted by @mbolivar in #2288 (comment)
The text was updated successfully, but these errors were encountered: