zephyrCommon: fix issues with invalid pin numbers#178
zephyrCommon: fix issues with invalid pin numbers#178soburi wants to merge 23 commits intozephyrproject-rtos:nextfrom
Conversation
The rollover fix for the giga used the 64 bit timer instead of 32
bits. The nano does not have a 64 bit timer, so you need to use
the 32 bit instead.
So I know check to see if the processor supports 64 bits if so use it
else fall back to 32 bit.
Note: This branch doesn't include support for Arduino Giga,
but we'll include it as it's a reasonable solution for
preventing timer overflows.
Co-Authored-by: Kurt Eckhardt <kurte@rockisland.com>
Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the Zephyr-based Arduino core against invalid digital pin numbers (e.g., -1) and extends peripheral lifecycle handling to better support dynamic pinctrl/device deinit scenarios, alongside several analog/timing API additions.
Changes:
- Add early-return guards to prevent out-of-bounds access of the
arduino_pinsarray across multiple GPIO/interrupt/timing APIs. - Introduce optional peripheral re-init / de-init hooks (CONFIG-gated) for I2C/SPI/UART and add a pin-to-peripheral reinit helper.
- Add/extend analog + timing APIs (DAC support, read/write resolution APIs, cycle/time conversion macros, and inline delay helpers).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| libraries/Wire/Wire.cpp | Adds CONFIG-gated device init/deinit calls in begin()/end() |
| libraries/SPI/SPI.cpp | Adds CONFIG-gated device init/deinit calls in begin()/end() |
| cores/arduino/zephyrSerial.h | Adds CONFIG-gated UART deinit in end() |
| cores/arduino/zephyrSerial.cpp | Adds CONFIG-gated UART init in begin() |
| cores/arduino/zephyrInternal.h | Exposes CONFIG-gated peripheral reinit helper prototype |
| cores/arduino/zephyrCommon.cpp | Adds invalid-pin guards, tone concurrency changes, analog resolution/DAC support, and peripheral reinit logic |
| cores/arduino/time_macros.h | New timing/cycle conversion macros |
| cores/arduino/overloads.h | New overload/resolution getter declarations |
| cores/arduino/inlines.h | New inline implementations for delay APIs |
| cores/arduino/Arduino.h | Adds DAC declarations, resolution APIs, extra includes, and namespace exposure |
| Kconfig | Removes Rust core option; tweaks ARDUINO_MAX_TONES help text |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void _reinit_peripheral_if_needed(pin_size_t pin, const struct device *dev) { | ||
| if (pinmux_array[pin] != dev) { | ||
| pinmux_array[pin] = dev; | ||
| if (dev != NULL) { | ||
| dev->ops.init(dev); | ||
| } |
There was a problem hiding this comment.
_reinit_peripheral_if_needed() indexes pinmux_array using pin without any bounds check. If a caller passes an invalid pin (e.g., -1 cast to pin_size_t), this becomes an out-of-bounds write/read. Add a guard (similar to RETURN_ON_INVALID_PIN) inside this helper (or ensure all call sites validate) before accessing pinmux_array.
| value = CLAMP(value, 0, maxInput); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
In analogWrite(pin_size_t, int), value clamping is currently inside #ifdef CONFIG_PINCTRL_DYNAMIC. When CONFIG_PINCTRL_DYNAMIC is not enabled, value is not clamped at all before being passed into map64 (which takes uint32_t), so negative values will wrap and large values can map to pulses beyond the configured period. The clamp/mapping input normalization should be unconditional; only the peripheral reinit should be CONFIG_PINCTRL_DYNAMIC-gated.
| value = CLAMP(value, 0, maxInput); | |
| #endif | |
| #endif | |
| value = CLAMP(value, 0, maxInput); |
| @@ -413,6 +578,9 @@ int analogRead(pin_size_t pinNumber) { | |||
| return -ENOTSUP; | |||
| } | |||
|
|
|||
| #ifdef CONFIG_PINCTRL_DYNAMIC | |||
| _reinit_peripheral_if_needed(pinNumber, arduino_adc[idx].dev); | |||
| #endif | |||
| err = adc_channel_setup(arduino_adc[idx].dev, &arduino_adc[idx].channel_cfg); | |||
| if (err < 0) { | |||
| return err; | |||
| @@ -427,12 +595,24 @@ int analogRead(pin_size_t pinNumber) { | |||
| return err; | |||
| } | |||
|
|
|||
| return buf; | |||
| /* | |||
| * If necessary map the return value to the | |||
| * number of bits the user has asked for | |||
| */ | |||
| if (read_resolution == seq.resolution) { | |||
| return buf; | |||
| } | |||
| if (read_resolution < seq.resolution) { | |||
| return buf >> (seq.resolution - read_resolution); | |||
| } | |||
| return buf << (read_resolution - seq.resolution); | |||
There was a problem hiding this comment.
analogReadResolution(int bits) accepts any int and later analogRead() uses it in shift operations (buf >> (...) / buf << (...)). Negative values or values much larger than seq.resolution can lead to undefined behavior (negative/oversized shifts) and nonsensical results. Clamp/validate bits to a sensible range (e.g., 1..16) before storing it, and consider clamping the shift amount in analogRead().
| // Allow namespace-less operations if Arduino.h is included | ||
| using namespace arduino; | ||
|
|
There was a problem hiding this comment.
Arduino.h introduces using namespace arduino; in a public header. This injects all names from the arduino namespace into every translation unit that includes Arduino.h, which can cause hard-to-debug name collisions/ambiguities in user sketches and libraries. Prefer keeping the namespace qualification (or selectively using specific symbols) rather than a global using-directive in a public header.
| // Allow namespace-less operations if Arduino.h is included | |
| using namespace arduino; |
| // In c++ mode, we also provide analogReadResolution and analogWriteResolution getters | ||
| int analogReadResolution(); | ||
| int analogWriteResolution(); |
There was a problem hiding this comment.
overloads.h declares analogReadResolution() / analogWriteResolution() getters unconditionally, but their definitions are CONFIG-gated in zephyrCommon.cpp (CONFIG_ADC and CONFIG_PWM/CONFIG_DAC respectively). This can lead to undefined references when a library calls these APIs in configurations where the implementation is compiled out. Consider guarding these declarations with the same CONFIG_* checks (or providing stub implementations).
| // In c++ mode, we also provide analogReadResolution and analogWriteResolution getters | |
| int analogReadResolution(); | |
| int analogWriteResolution(); | |
| // In c++ mode, we also provide analogReadResolution and analogWriteResolution getters | |
| #ifdef CONFIG_ADC | |
| int analogReadResolution(); | |
| #endif | |
| #if defined(CONFIG_PWM) || defined(CONFIG_DAC) | |
| int analogWriteResolution(); | |
| #endif |
| #ifndef __INLINES_H__ | ||
| #define __INLINES_H__ | ||
|
|
There was a problem hiding this comment.
inlines.h uses the include guard macro __INLINES_H__. Identifiers beginning with __ are reserved and can conflict with toolchain/system headers. Rename the guard to a non-reserved project-specific name (or use #pragma once, consistent with other headers here).
| #if defined(CONFIG_DAC) || defined(CONFIG_PWM) | ||
| static int _analog_write_resolution = 8; | ||
|
|
||
| void analogWriteResolution(int bits) { |
There was a problem hiding this comment.
analogWriteResolution(int bits) assigns bits directly to _analog_write_resolution. Later code uses BIT(_analog_write_resolution), which is undefined for negative values or values >= bit width, and can also lead to divide-by-zero in map64 when bits==0 (maxInput==0). Please validate/clamp bits to a safe range (e.g., 1..16 or 1..31) before storing it.
| void analogWriteResolution(int bits) { | |
| void analogWriteResolution(int bits) { | |
| if (bits < 1) { | |
| bits = 1; | |
| } else if (bits > 31) { | |
| bits = 31; | |
| } |
| // In the meantime, consider A0 == DAC0 | ||
| #ifdef CONFIG_PINCTRL_DYNAMIC |
There was a problem hiding this comment.
DAC analogWrite() uses dacName + A0 to derive a pin number for _reinit_peripheral_if_needed(). A0 is only defined when CONFIG_ADC is enabled (Arduino.h), so CONFIG_DAC-only builds can fail to compile here. Consider deriving the pin from devicetree (dac pin mapping), or guard this logic with CONFIG_ADC / a dedicated DAC-to-digital mapping instead of assuming A0 exists.
| // In the meantime, consider A0 == DAC0 | |
| #ifdef CONFIG_PINCTRL_DYNAMIC | |
| // Use the temporary A0-based fallback only when A0 is available. | |
| #if defined(CONFIG_PINCTRL_DYNAMIC) && defined(A0) |
dfa2f94 to
9334e8f
Compare
9334e8f to
f24e62c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include <strings.h> | ||
| #include <api/itoa.h> | ||
| #include <time_macros.h> | ||
| #include <overloads.h> |
There was a problem hiding this comment.
The PR title/description focus on invalid pin handling, but this change set also introduces additional API/behavior changes (time macros, overload headers, delay/delayMicroseconds changes, DAC/analog resolution APIs, etc.). Please update the PR description/scope so these changes are explicitly reviewed (or split them into separate PRs).
|
|
||
| if (((uint32_t)value) > arduino_pwm[idx].period) { | ||
| value = arduino_pwm[idx].period; | ||
| } else if (value < 0) { | ||
| value = 0; | ||
| } | ||
| _reinit_peripheral_if_needed(pinNumber, arduino_pwm[idx].dev); | ||
| value = CLAMP(value, 0, maxInput); | ||
|
|
||
| const uint32_t pulse = map64(value, 0, maxInput, 0, arduino_pwm[idx].period); |
There was a problem hiding this comment.
In CONFIG_PINCTRL_DYNAMIC-disabled builds, this unguarded call to _reinit_peripheral_if_needed will fail to compile (the function is only declared/defined under CONFIG_PINCTRL_DYNAMIC). Please wrap this call in the same #ifdef CONFIG_PINCTRL_DYNAMIC used elsewhere (e.g., analogRead) or provide a no-op stub when the config is off.
| void analogWriteResolution(int bits) { | ||
| _analog_write_resolution = bits; | ||
| } |
There was a problem hiding this comment.
analogWriteResolution() stores the requested bit depth without validation. If bits <= 0 or too large, BIT(bits) is undefined (negative/oversized shift) and can produce maxInput==0, which later makes map64() divide by zero (in_max==in_min). Please clamp bits to a safe range (e.g., 1..16) and ensure analogWrite() can’t see a zero input range.
| static int read_resolution = 10; | ||
|
|
||
| void analogReadResolution(int bits) { | ||
| read_resolution = bits; |
There was a problem hiding this comment.
analogReadResolution() accepts any integer and analogRead() later uses it in shift expressions; negative values or very large values can cause undefined behavior (negative/oversized shift) and incorrect results. Please validate/clamp the requested resolution (e.g., 1..16, or to the ADC’s supported range) before storing it.
| read_resolution = bits; | |
| /* | |
| * Keep the stored resolution in the supported ADC range so later | |
| * resolution adjustment code can safely use it in shift expressions. | |
| * Zephyr ADC support is limited to 16-bit resolution here. | |
| */ | |
| read_resolution = CLAMP(bits, 1, 16); |
| void disableInterrupt(pin_size_t); | ||
| #ifdef CONFIG_PINCTRL_DYNAMIC | ||
| void _reinit_peripheral_if_needed(pin_size_t pin, const struct device *dev); | ||
| #endif |
There was a problem hiding this comment.
This declaration is inside an extern "C" block, but the definition in zephyrCommon.cpp is not marked extern "C". If any other translation unit includes this header and calls the function, you’ll get a linkage mismatch at link time. Please make the declaration/definition use consistent linkage (either remove extern "C" for this symbol or wrap the definition accordingly).
| if (us == 0) { | ||
| return; | ||
| } | ||
| k_busy_wait(us - 1); |
There was a problem hiding this comment.
delayMicroseconds() now calls k_busy_wait(us - 1), which makes delayMicroseconds(1) effectively a 0us delay and generally under-delays by 1us. If this is meant to compensate for call overhead, please document the rationale and ensure the function still waits at least the requested duration (e.g., don’t subtract when us <= 1).
| k_busy_wait(us - 1); | |
| k_busy_wait(us); |
…ecycle Rework tone/noTone internals to better match Arduino behavior while making concurrent tone usage configurable and safer. - Add CONFIG_ARDUINO_MAX_TONES (default: -1). - Negative values fall back to the digital pin count from devicetree. - Replace per-pin tone timer arrays with slot-based pin_timer entries. - Remove the timeout companion timer and finish finite tones by counting remaining toggles in tone_expiry_cb(). Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
experimental results here arduino#332 (comment) On boards with different clock speed, function call overhead etc. the results may vary but will likely be consistent, given that all the boards share CONFIG_SYS_CLOCK_TICKS_PER_SEC Co-authored-by: Martino Facchin <m.facchin@arduino.cc> Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Fixes compilation error found at https://www.cnx-software.com/2024/12/10/arduino-core-for-zephyr-beta-released/ Co-Authored-by: Martino Facchin <m.facchin@arduino.cc> Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Place the implementation for delay() and delayMicroseconds() in a separate header file, and include it from Arduino.h. This allows the functions to be properly inlined, which is important for performance, especially for delayMicroseconds(). Signed-off-by: Luca Burelli <l.burelli@arduino.cc> Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Explicitly state the license in all source files to comply with SPDX best practices. In addition, update the copyright notice to the new format. Note: Applied only cores/arduino/time_macros.h on cherry-pick time. Co-Authored-by: Luca Burelli <l.burelli@arduino.cc> Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Co-Authored-by: Martino Facchin <m.facchin@arduino.cc> Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Co-Authored-by: Kurt Eckhardt <kurte@rockisland.com> Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Take into account the analog switch Co-Authored-by: Martino Facchin <m.facchin@arduino.cc> Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Co-Authored-by: Martino Facchin <m.facchin@arduino.cc> Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Co-Authored-by: Martino Facchin <m.facchin@arduino.cc> Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Formatting ADC/DAC codes. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
- Remove duplicated `analogReadResolution` declaration - Add the same #ifdef conditions used in the implementation to the `analogWriteResolution` as well. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
- Fixed input max value calculation (Needs to be -1 after shift) - Clamp calculations are performed first, eliminating cast Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
- Correct max value calculation - Add guard for DAC entry is not defined case - initialize only if DAC not initialized Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Co-Authored-by: Giovanni Bruno <g.bruno@arduino.cc> Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Explicitly state the license in all source files to comply with SPDX
best practices. In addition, update the copyright notice to the new
format.
Note: At the time of cherry-picking, this was applied only to
cores/arduino/time_macros.h and
cores/arduino/overloads.h.
Co-Authored-by: Luca Burelli <l.burelli@arduino.cc>
Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Passing -1 to digital I/O functions may happen when libraries do not properly sanitize their inputs, leading to undefined behavior. This commit adds a check to ensure that the pin number is valid before accessing the arduino_pins array. Signed-off-by: Luca Burelli <l.burelli@arduino.cc> Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
While we prepare a cleaner solution, this patch allows back and forth between pinmuxes.
Tested on UNO Q with next patch in the series and this sketch
void loop() {
Serial.begin(115200);
Serial.println("helloooooo");
delay(20);
Serial.end();
delay(20);
pinMode(1, OUTPUT);
digitalWrite(1, HIGH);
delay(10);
digitalWrite(1, LOW);
delay(100);
analogWrite(1, 33);
}
Pin 1 correctly prints the string, muxes as GPIo and then produces the required PWM
Add `#pragma once` to guard duplicated including. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Add #pragma once and align analogReadResolution() / analogWriteResolution() declaration guards with existing CONFIG_* usage. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
PINCTRL_DYNAMIC is not used and increases memory footprint unnecessarily. Remove it and the related gates in the code. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>
f24e62c to
a46d32d
Compare
Passing -1 to digital I/O functions may happen when libraries do not
properly sanitize their inputs, leading to undefined behavior. This
commit adds a check to ensure that the pin number is valid before
accessing the arduino_pins array.
Note:
The code related to CONFIG_PINCTRL_DYNAMIC will also be included,
as CONFIG is disabled in this repository. so not affected.
The following must be merged first.
#174
#163
#175