-
Notifications
You must be signed in to change notification settings - Fork 55
[DNM] zephyrCommon: Changing pin numbering rules #134
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
base: next
Are you sure you want to change the base?
Conversation
93e11fa to
084197c
Compare
0747e08 to
014a6ed
Compare
7ca4da5 to
b009050
Compare
26d967e to
bb1ef24
Compare
bb1ef24 to
e8dced1
Compare
|
Just chiming in for the Arduino side to say, please don't drop custom pin numbering. The technical implementation is great 👍 and it removes the need for bulky pin lists 💛, but AFAICS also makes it "impossible" to assign specific numbers to pins, and this is something that is really important for backwards compatibility with traditional Arduino sketches. [ OTOH, I do hate having to write those lists; tried several alternative DTS mappings myself but haven't yet found one that is flexible enough and/or enough of an improvement... 🙁 ] |
The main purpose of this change is to make pins that are not exposed on the connector usable. By the way, zephyrproject-rtos/zephyr#87595 is stalled because only a few people reviewed it. |
e8dced1 to
199421c
Compare
199421c to
934a931
Compare
Does this mean you want to freely map the direct values passed to digitalOut? I understand that traditionally, Arduino's D1-based values were logical pin numbers, Until now, the gsoc-2022-arduino-core implementation has assumed logical = physical due to various constraints. (i.e., D0 = 0, D1 = 1) In this implementation, only mapped pins are usable, so the intention of this PR is to introduce physical pin numbers to handle all pins on the board. If the answer to the initial question is yes, I think this can be addressed by adding one more LUT. |
DhruvaG2000
left a comment
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 have seen the PR, and have some catching up to do on the new zephyr commit that has been mentioned. I will need some more time to actually go through since this seems to have a large impact overall. Please excuse the delay!
| return sum_of_list(sum + head, tail...); | ||
| constexpr inline size_t port_index_r(const struct device *target, | ||
| const struct device *const *table, pin_size_t idx, size_t n) { | ||
| return (n == 0) ? size_t(-1) |
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.
Is it me or is the alignment really messed up on this one?
DhruvaG2000
left a comment
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.
@Ayush1325 if possible can you test for any regressions on the Beagleconnect freedom?
I am ok with the general idea here, and it does seem like an improvement for handling gpio ports.
Just need to be careful about the code though, and hence giving some comments from maintenance and debugging perspective.
|
I'll merge this as soon as I am done fixing the CI failures. Please excuse the delay |
Apologies for the late reply. Seems to be working fine on bcf. |
|
@soburi, apologies for getting back so late, was derailed by the linked PR and most importantly all of this week's chaos. 😅
In fact, and very unfortunately, the "official" way of referring to pins is by number. Some cores don't even define
Understood; with this change you assign a number for any "GPIO bit" (even if unconnected or not even exposed by the SOC!), and then map
My issue with a LUT is that it will create two sets of indistinguishable integers : the "logical" ones (inputs to the LUT) and the "physical" ones (after the LUT is applied). Now, 🥁, when a sketch then calls We have been bitten HARD by this with the Arduino Nano ESP32 - the GPIO controller there already uses numbers, but we need to map them to keep "standard Nano" pin layout. We added a LUT so that APIs would input logical and address physical. Chaos ensued, and 2 years later this is still a raw topic on the Arduino Forums. (brainstorming) There MAY be a way out. The default index pin type
(all of this needs a constant LUT, probably magically generated from the DT in some way. Would the recent Let me know what you guys think! |
934a931 to
4abaf6a
Compare
|
Sorry for my late response.
I know it was a difficult situation. You must be tired.
I understood what that means is "all is device dependent'.
In my opinion, they are distinct clearly. First, there is a need to clarify the terminology. physical pin number: The number statically derived from the hardware configuration. The 'serialized GPIO pin number' in the my proposal can be derived from the GPIO port and the pin number, so it is a type of "physical pin number". Logical Pin Number = The number defined by the rule. digitalWrite() (And all other APIs) must take a logical pin number. If we keep this principle, it will not cause a serious problem.
I couldn't understand the details, I need to check a little more carefully.
This is the principal.
On that basis,
Zephyr's pin numbers are specified in 32 bits, so I don't think there's any problem with this policy. |
Zephyr's ba48d83b changes make it easy to configure pin numbering at compile, so we'll change the pin numbers to be based on the GPIO pin numbers. This is same as the traditional Arduino, using the GPIO numbers as pin numbers. Pin names written on the board, such as D1 and D2, are aliases for these pin numbers. When using multiple GPIO ports, pin numbers are numbered consecutively from the beginning. For example, if you have two 16-port GPIOs, 16 refers to the first pin (idx=0) of the second port. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Now that GPIO can be specified generically, we will discontinue the definition generation of LED_BUILTIN from builtin_led_gpios. Delete this definition where automatic generation from led0 is not an issue, and define it in variant.h in all other cases. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
4abaf6a to
e9c47e4
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.
Pull request overview
This PR refactors the pin numbering system in the Zephyr Arduino core to align with recent Zephyr upstream changes. The new approach uses GPIO pin numbers directly as Arduino pin numbers, similar to traditional Arduino. Pin aliases (like D1, D2) now reference these GPIO-based pin numbers instead of using a separate indexing scheme.
Key Changes:
- Replaced the
builtin-led-gpiosdevicetree property with variant-specificLED_BUILTINdefinitions using pin aliases (e.g.,D13) - Added
gpio-portsproperty to overlay files to specify which GPIO ports are used for pin numbering - Refactored pin lookup logic from devicetree-based arrays to compile-time calculations based on GPIO port offsets
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| variants/*/*.overlay | Added gpio-ports property and removed builtin-led-gpios property for each board variant |
| variants/cc3220sf_launchxl_cc3220sf/variant.h | Updated LED definitions to use pin aliases (D4, D3, D19) instead of numeric constants |
| variants/arduino_nano_33_ble_*/variant.h | Added LED_BUILTIN definition using D13 alias |
| cores/arduino/zephyrCommon.cpp | Completely refactored GPIO pin mapping from array lookups to constexpr functions that calculate port/pin from GPIO numbering |
| cores/arduino/Arduino.h | Refactored macro definitions for pin enumerations and LED_BUILTIN to work with new GPIO-based numbering scheme |
| documentation/variants.md | Updated documentation to describe new gpio-ports configuration and LED_BUILTIN behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| The `gpios` node lists the GPIO ports to use. | ||
| Pin numbers will be assigned sequentially starting from 0 to the GPIOs of the ports specified here. | ||
| For example, if gpio0 and gpio1, each with 16 outputs, are specified, pin 1 of gpio1 will be assigned 17. |
Copilot
AI
Dec 19, 2025
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.
The example code shows pin 1 of gpio1 being assigned pin number 17, but the description states it should be 16. If gpio0 has pins 0-15 (16 pins total), then the first pin (idx=0) of gpio1 would be pin 16, not 17. Please update either the example or the explanation for consistency.
| For example, if gpio0 and gpio1, each with 16 outputs, are specified, pin 1 of gpio1 will be assigned 17. | |
| For example, if gpio0 and gpio1, each with 16 outputs, are specified, pin 0 of gpio1 will be assigned 16. |
| } | ||
|
|
||
| constexpr inline pin_size_t global_gpio_pin_(size_t port_idx, pin_size_t lpin) { | ||
| return port_idx == size_t(-1) ? size_t(-1) : end_accum(port_idx) + lpin; |
Copilot
AI
Dec 19, 2025
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.
In the global_gpio_pin_ function, the return type is pin_size_t but it returns size_t(-1) on error. This is inconsistent with the function signature and could cause issues. Either change the error value to pin_size_t(-1) or update the return type to size_t.
| return port_idx == size_t(-1) ? size_t(-1) : end_accum(port_idx) + lpin; | |
| return port_idx == size_t(-1) ? pin_size_t(-1) : end_accum(port_idx) + lpin; |
| gpio_pin_set(local_gpio_port(pt->pin), local_gpio_pin(pt->pin), 0); | ||
| } else { | ||
| gpio_pin_toggle_dt(spec); | ||
| gpio_pin_toggle(local_gpio_port(pt->pin), local_gpio_pin(pt->pin)); |
Copilot
AI
Dec 19, 2025
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.
Missing error handling. These function calls don't check if local_gpio_port(pinNumber) returns nullptr before dereferencing, which could lead to crashes if an invalid pin number is provided.
| unsigned long pulseIn(pin_size_t pinNumber, uint8_t state, unsigned long timeout) { | ||
| const struct device* port = local_gpio_port(pinNumber); | ||
| const size_t pin = local_gpio_pin(pinNumber); | ||
| struct k_timer timer; | ||
| int64_t start, end, delta = 0; | ||
| const struct gpio_dt_spec *spec = &arduino_pins[pinNumber]; | ||
|
|
||
| if (!gpio_is_ready_dt(spec)) { | ||
| if (!device_is_ready(port)) { | ||
| return 0; | ||
| } | ||
|
|
||
| k_timer_init(&timer, NULL, NULL); | ||
| k_timer_start(&timer, K_MSEC(timeout), K_NO_WAIT); | ||
|
|
||
| while(gpio_pin_get_dt(spec) == state && k_timer_status_get(&timer) == 0); | ||
| while (gpio_pin_get(port, pin) == state && k_timer_status_get(&timer) == 0); | ||
| if (k_timer_status_get(&timer) > 0) { | ||
| goto cleanup; | ||
| } | ||
|
|
||
| while(gpio_pin_get_dt(spec) != state && k_timer_status_get(&timer) == 0); | ||
| while (gpio_pin_get(port, pin) != state && k_timer_status_get(&timer) == 0); | ||
| if (k_timer_status_get(&timer) > 0) { | ||
| goto cleanup; | ||
| } | ||
|
|
||
| start = k_uptime_ticks(); | ||
| while(gpio_pin_get_dt(spec) == state && k_timer_status_get(&timer) == 0); | ||
| while (gpio_pin_get(port, pin) == state && k_timer_status_get(&timer) == 0); | ||
| if (k_timer_status_get(&timer) > 0) { | ||
| goto cleanup; | ||
| } |
Copilot
AI
Dec 19, 2025
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.
Missing error handling. The port variable is obtained from local_gpio_port(pinNumber) which could return nullptr. While there's a device_is_ready check, if port is nullptr, this would still cause undefined behavior.
| void enableInterrupt(pin_size_t pinNumber) { | ||
| struct gpio_port_callback *pcb = find_gpio_port_callback(arduino_pins[pinNumber].port); | ||
| struct gpio_port_callback *pcb = find_gpio_port_callback(local_gpio_port(pinNumber)); | ||
|
|
||
| if (pcb) { | ||
| pcb->handlers[arduino_pins[pinNumber].pin].enabled = true; | ||
| pcb->handlers[local_gpio_pin(pinNumber)].enabled = true; | ||
| } | ||
| } | ||
|
|
||
| void disableInterrupt(pin_size_t pinNumber) { | ||
| struct gpio_port_callback *pcb = find_gpio_port_callback(arduino_pins[pinNumber].port); | ||
| struct gpio_port_callback *pcb = find_gpio_port_callback(local_gpio_port(pinNumber)); | ||
|
|
||
| if (pcb) { | ||
| pcb->handlers[arduino_pins[pinNumber].pin].enabled = false; | ||
| pcb->handlers[local_gpio_pin(pinNumber)].enabled = false; | ||
| } |
Copilot
AI
Dec 19, 2025
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.
Missing error handling. local_gpio_port(pinNumber) could return nullptr, which would cause issues when passed to find_gpio_port_callback.
| The `gpios` node lists the GPIO ports to use. | ||
| Pin numbers will be assigned sequentially starting from 0 to the GPIOs of the ports specified here. | ||
| For example, if gpio0 and gpio1, each with 16 outputs, are specified, pin 1 of gpio1 will be assigned 17. | ||
|
|
||
| ``` | ||
| / { | ||
| zephyr,user { | ||
| gpios = <&gpio0>, <&gpio1>; | ||
| }; | ||
| }; | ||
| ``` |
Copilot
AI
Dec 19, 2025
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.
The documentation mentions a node named gpios, but the actual overlay files use gpio-ports. The documentation should use the correct property name gpio-ports to match the implementation.
| constexpr inline const struct device *local_gpio_port_r(pin_size_t pin, | ||
| const struct device *const *ctrl, | ||
| const uint32_t accum, | ||
| const uint32_t *end, size_t n) { |
Copilot
AI
Dec 19, 2025
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.
Inconsistent spacing in function parameters. The parameter list has inconsistent tab/space alignment. The third parameter starts at a different column than the others, making the code harder to read.
| const uint32_t *end, size_t n) { | |
| const uint32_t *end, size_t n) { |
| void pinMode(pin_size_t pinNumber, PinMode pinMode) { | ||
| if (pinMode == INPUT) { // input mode | ||
| gpio_pin_configure_dt(&arduino_pins[pinNumber], | ||
| GPIO_INPUT | GPIO_ACTIVE_HIGH); | ||
| } else if (pinMode == INPUT_PULLUP) { // input with internal pull-up | ||
| gpio_pin_configure_dt(&arduino_pins[pinNumber], | ||
| GPIO_INPUT | GPIO_PULL_UP | GPIO_ACTIVE_HIGH); | ||
| } else if (pinMode == INPUT_PULLDOWN) { // input with internal pull-down | ||
| gpio_pin_configure_dt(&arduino_pins[pinNumber], | ||
| GPIO_INPUT | GPIO_PULL_DOWN | GPIO_ACTIVE_HIGH); | ||
| } else if (pinMode == OUTPUT) { // output mode | ||
| gpio_pin_configure_dt(&arduino_pins[pinNumber], | ||
| GPIO_OUTPUT_LOW | GPIO_ACTIVE_HIGH); | ||
| if (pinMode == INPUT) { | ||
| global_gpio_pin_configure(pinNumber, GPIO_INPUT | GPIO_ACTIVE_HIGH); | ||
| } else if (pinMode == INPUT_PULLUP) { | ||
| global_gpio_pin_configure(pinNumber, GPIO_INPUT | GPIO_PULL_UP | GPIO_ACTIVE_HIGH); | ||
| } else if (pinMode == INPUT_PULLDOWN) { | ||
| global_gpio_pin_configure(pinNumber, GPIO_INPUT | GPIO_PULL_DOWN | GPIO_ACTIVE_HIGH); | ||
| } else if (pinMode == OUTPUT) { | ||
| global_gpio_pin_configure(pinNumber, GPIO_OUTPUT_LOW | GPIO_ACTIVE_HIGH); | ||
| } |
Copilot
AI
Dec 19, 2025
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.
Missing error handling. The functions local_gpio_port, local_gpio_pin, and global_gpio_pin_configure can return error values (nullptr or -1), but they are used without checking the return value. This could lead to dereferencing a null pointer or operating on an invalid pin number when an invalid pin is provided.
| pcb = find_gpio_port_callback(local_gpio_port(pinNumber)); | ||
| __ASSERT(pcb != nullptr, "gpio_port_callback not found"); | ||
|
|
||
| pcb->pins |= BIT(arduino_pins[pinNumber].pin); | ||
| pcb->pins |= BIT(local_gpio_pin(pinNumber)); | ||
| setInterruptHandler(pinNumber, callback); | ||
| enableInterrupt(pinNumber); | ||
|
|
||
| gpio_pin_interrupt_configure(arduino_pins[pinNumber].port, arduino_pins[pinNumber].pin, intmode); | ||
| gpio_pin_interrupt_configure(local_gpio_port(pinNumber), local_gpio_pin(pinNumber), intmode); | ||
| gpio_init_callback(&pcb->callback, handleGpioCallback, pcb->pins); | ||
| gpio_add_callback(arduino_pins[pinNumber].port, &pcb->callback); | ||
| gpio_add_callback(local_gpio_port(pinNumber), &pcb->callback); |
Copilot
AI
Dec 19, 2025
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.
Missing error handling. local_gpio_port(pinNumber) could return nullptr, which would cause undefined behavior when passed to GPIO functions.
| int digitalPinToInterrupt(pin_size_t pin) { | ||
| struct gpio_port_callback *pcb = | ||
| find_gpio_port_callback(arduino_pins[pin].port); | ||
| find_gpio_port_callback(local_gpio_port(pin)); | ||
|
|
||
| return (pcb) ? pin : -1; | ||
| } |
Copilot
AI
Dec 19, 2025
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.
Missing error handling. local_gpio_port(pin) could return nullptr, which would cause issues in find_gpio_port_callback.
Zephyr's ba48d83b
changes make it easy to configure pin numbering at compile,
so we'll change the pin numbers to be based on the GPIO pin numbers.
This is same as the traditional Arduino, using the GPIO numbers as
pin numbers. Pin names written on the board, such as D1 and D2,
are aliases for these pin numbers.
When using multiple GPIO ports, pin numbers are numbered consecutively
from the beginning. For example, if you have two 16-port GPIOs,
16 refers to the first pin (idx=0) of the second port.