Conversation
There was a problem hiding this comment.
Pull request overview
Introduces an alternative Arduino API path that leans on Rust-provided enum types (via cbindgen) and a protobuf-based IDL pipeline (nanopb + custom protoc plugin) to generate C++ interface headers for Arduino classes.
Changes:
- Add Arduino IDL
.protofiles and a customprotoc-gen-arduinoifgenerator to emit*_ifc.hppinterfaces. - Add Rust
arduino_typesenums and a CMake+cbindgen step to generatearduino_types.hfor C/C++ consumers. - Add/adjust Zephyr Arduino API headers and sample configs to enable the Rust implementation path.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/protoc-gen-arduinoif | New protoc plugin to generate C++ interface headers from service definitions/options. |
| samples/hello_arduino/prj.conf | Enable Rust Arduino API implementation in sample config. |
| samples/blinky_arduino/prj.conf | Enable Rust Arduino API implementation in sample config. |
| rust/src/lib.rs | Export new arduino_types module from the Rust crate. |
| rust/src/arduino_types/mod.rs | Module wiring/re-exports for Rust Arduino enum types. |
| rust/src/arduino_types/common.rs | Rust enums for common Arduino concepts (pins, bit order). |
| rust/src/arduino_types/hardware_spi.rs | Rust enums for SPI mode/bus mode. |
| rust/src/arduino_types/stream.rs | Rust enum for stream lookahead mode. |
| rust/src/arduino_types/ip_address.rs | Rust enum for IP address type. |
| rust/src/arduino_types/hardware_can.rs | Rust enum for CAN bit rates. |
| rust/src/arduino_types/hardware_serial.rs | Rust enums for serial parity/stop bits/data length masks. |
| rust/cbindgen.toml | cbindgen configuration to export selected Rust enums into a C header. |
| idl/CMakeLists.txt | Build integration for nanopb + protoc plugin + generated interface headers. |
| idl/proto/arduino_opts.proto | Defines protobuf custom options used to carry C++ signature metadata. |
| idl/proto/print.proto | IDL for Print service interface. |
| idl/proto/stream.proto | IDL for Stream service interface. |
| idl/proto/client.proto | IDL for Client service interface. |
| idl/proto/server.proto | IDL for Server service interface. |
| idl/proto/hardware_serial.proto | IDL for HardwareSerial service interface. |
| idl/proto/hardware_i2c.proto | IDL for HardwareI2C service interface. |
| idl/proto/hardware_spi.proto | IDL for HardwareSPI service interface. |
| idl/proto/hardware_can.proto | IDL for HardwareCAN service interface. |
| idl/proto/udp.proto | IDL for UDP service interface. |
| idl/proto/pluggable_usb_module.proto | IDL for PluggableUSBModule service interface. |
| idl/proto/printable.proto | IDL for Printable service interface. |
| idl/proto/google/protobuf/empty.proto | Local minimal google.protobuf.Empty used by the IDL protos. |
| idl/proto/google/protobuf/wrappers.proto | Local minimal wrapper messages used by the IDL protos. |
| cores/arduino/zephyr/api/Print.h | New Print class header, now tied to generated PrintIfc. |
| cores/arduino/zephyr/api/Stream.h | New Stream class header, now tied to generated StreamIfc. |
| cores/arduino/zephyr/api/HardwareSerial.h | New HardwareSerial header and SerialConfig constants. |
| cores/arduino/zephyr/api/HardwareI2C.h | Stub HardwareI2C header. |
| cores/arduino/zephyr/api/ArduinoAPI.h | New Arduino API surface header for the Zephyr/Rust implementation path. |
| cores/arduino/zephyr/api/Common.h | Placeholder header. |
| cores/arduino/apiCommon.cpp | Previous Arduino common wrappers removed (file now empty). |
| cores/arduino/zephyrPrint.h | Previous inline Print helpers removed (file now empty). |
| Kconfig | Rust implementation now selects NANOPB. |
| CMakeLists.txt | Add cbindgen header generation + include path, and include the new idl/ subdirectory for Rust builds. |
Comments suppressed due to low confidence (2)
cores/arduino/apiCommon.cpp:1
- This source file is now empty. If its functionality has moved elsewhere, it should be removed from the tree (and from any build/source lists) to avoid compiling an empty translation unit and confusing future maintenance.
cores/arduino/zephyrPrint.h:1 - This header is now empty. If it’s no longer used, remove it (and any references) to avoid leaving dead/placeholder headers in the public API surface.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _cpp_namespace_from_package(package_name: str) -> str: | ||
| return "::".join(part for part in package_name.split(".") if part) | ||
|
|
||
|
|
||
| def _generate_service_header( | ||
| service, | ||
| message_map: Dict[str, DescriptorProto], | ||
| package_name: str, | ||
| ) -> str: | ||
| class_name = f"{service.name}Ifc" | ||
| namespace_name = _cpp_namespace_from_package(package_name) | ||
| lines: List[str] = [] | ||
|
|
||
| lines.append("// SPDX-License-Identifier: Apache-2.0") | ||
| lines.append("// Generated by protoc-gen-arduinoif. DO NOT EDIT.") | ||
| lines.append("") | ||
| lines.append("#pragma once") | ||
| lines.append("") | ||
| lines.append("#include <cstddef>") | ||
| lines.append("#include <cstdint>") | ||
| for include in SERVICE_EXTRA_INCLUDES.get(service.name, []): | ||
| lines.append(f"#include <{include}>") | ||
| lines.append("") | ||
| if namespace_name: | ||
| lines.append(f"namespace {namespace_name} {{") | ||
| lines.append("") |
There was a problem hiding this comment.
_cpp_namespace_from_package() emits namespaces using the C++17 nested-namespace syntax (e.g., namespace arduino::idl {). The existing C++ code in this repo uses the pre-C++17 style (namespace arduino { namespace zephyr { ... } }). If the build is not guaranteed to use C++17, generate nested namespace blocks instead to maintain compatibility.
| add_custom_command( | ||
| OUTPUT ${RUST_ENUM_HEADER} | ||
| COMMAND ${CBINDGEN_EXECUTABLE} | ||
| --config ${RUST_CBINDGEN_CONFIG} | ||
| --crate arduinocore_api_rust | ||
| --output ${RUST_ENUM_HEADER} | ||
| WORKING_DIRECTORY ${RUST_CRATE_DIR} | ||
| DEPENDS ${RUST_ENUM_SRCS} ${RUST_CBINDGEN_CONFIG} |
There was a problem hiding this comment.
The cbindgen custom command writes to ${RUST_ENUM_HEADER_DIR}/arduino_types.h but doesn’t ensure ${RUST_ENUM_HEADER_DIR} exists. Add a preceding cmake -E make_directory step to prevent build failures on clean builds.
| SERVICE_EXTRA_INCLUDES = { | ||
| "Client": ["api/IPAddress.h"], | ||
| "HardwareSPI": ["api/HardwareSPI.h"], | ||
| "HardwareCAN": ["api/HardwareCAN.h"], | ||
| "UDP": ["api/IPAddress.h"], | ||
| "PluggableUSBModule": ["api/PluggableUSB.h"], | ||
| "Printable": ["api/Print.h"], | ||
| } |
There was a problem hiding this comment.
SERVICE_EXTRA_INCLUDES references several headers that do not exist in this repository (e.g., api/IPAddress.h, api/HardwareSPI.h, api/HardwareCAN.h, api/PluggableUSB.h). Since these includes get emitted into the generated *_ifc.hpp headers, builds will fail as soon as those interfaces are generated/include. Either add these headers (even as minimal forward-decl stubs) under the configured include path or remove/replace the extra includes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| namespace arduino { | ||
|
|
||
| class Client : virtual public Stream, virtual public idl::ClientIfc { |
There was a problem hiding this comment.
Inconsistent interface reference. The file uses "ClientIfc" at line 9 but should use "idl::ClientIfc" for consistency with other headers that properly namespace the interface classes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[inline] | ||
| fn state_mut<'a>( | ||
| buffer: *mut *mut c_char, | ||
| capacity: *mut c_uint, | ||
| len: *mut c_uint, | ||
| ) -> Option<(&'a mut *mut c_char, &'a mut c_uint, &'a mut c_uint)> { | ||
| if buffer.is_null() || capacity.is_null() || len.is_null() { | ||
| None | ||
| } else { | ||
| // SAFETY: null is checked above and callers pass valid pointers. | ||
| Some(unsafe { (&mut *buffer, &mut *capacity, &mut *len) }) | ||
| } |
There was a problem hiding this comment.
state_mut is a safe function that manufactures &mut references from raw pointers and returns them with an unconstrained lifetime parameter ('a). This is unsound: callers could hold the returned references past the actual pointee lifetime, causing UB. Make state_mut unsafe and/or return raw pointers, and avoid exposing arbitrary lifetimes in the signature (let the borrow be scoped to the call site).
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn arduino_string_free( | ||
| buffer: *mut *mut c_char, | ||
| capacity: *mut c_uint, | ||
| len: *mut c_uint, | ||
| ) { | ||
| let Some((buffer, capacity, len)) = state_mut(buffer, capacity, len) else { | ||
| return; | ||
| }; | ||
|
|
||
| if !(*buffer).is_null() { | ||
| // SAFETY: buffer pointer comes from malloc/realloc. | ||
| unsafe { | ||
| free((*buffer).cast()); | ||
| } | ||
| } | ||
|
|
||
| *buffer = ptr::null_mut(); | ||
| *capacity = 0; | ||
| *len = 0; | ||
| } | ||
|
|
||
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn arduino_string_change_buffer( | ||
| buffer: *mut *mut c_char, | ||
| capacity: *mut c_uint, | ||
| len: *mut c_uint, | ||
| max_str_len: c_uint, | ||
| ) -> bool { |
There was a problem hiding this comment.
These exported FFI functions take raw pointers but are declared as safe pub extern "C" fn. Safe Rust code could call them with invalid pointers and trigger UB inside state_mut/dereferences. Mark these APIs pub unsafe extern "C" fn (or wrap the pointer handling behind a safe Rust API and keep the extern entrypoints unsafe).
| return_type = _get_string_option(method.options, METHOD_CPP_RETURN_TAG).strip() | ||
| if not return_type: | ||
| output_message = message_map.get(method.output_type) | ||
| if output_message is not None and len(output_message.field) == 0: | ||
| return_type = "void" | ||
| else: | ||
| return_type = "void" | ||
|
|
There was a problem hiding this comment.
In _method_spec_from_descriptor, the default return-type inference is currently a no-op: both branches set return_type = "void", so any RPC without an explicit arduino.cpp_decl/arduino.cpp_return will always generate a void method even if the output message is non-empty. Either remove the dead conditional or implement proper return-type mapping based on method.output_type (e.g., wrappers/value messages).
| set(RUST_ENUM_SRCS | ||
| ${RUST_CRATE_DIR}/src/lib.rs | ||
| ${RUST_CRATE_DIR}/src/string.rs | ||
| ${RUST_CRATE_DIR}/src/arduino_types/common.rs | ||
| ${RUST_CRATE_DIR}/src/arduino_types/hardware_spi.rs | ||
| ${RUST_CRATE_DIR}/src/arduino_types/stream.rs | ||
| ${RUST_CRATE_DIR}/src/arduino_types/ip_address.rs | ||
| ${RUST_CRATE_DIR}/src/arduino_types/hardware_can.rs |
There was a problem hiding this comment.
RUST_ENUM_SRCS is missing several Rust sources that affect both the cbindgen output and when CMake decides to rerun cbindgen/cargo build (e.g., src/arduino_types/mod.rs and src/arduino_types/hardware_serial.rs, which define enums listed in cbindgen.toml). This can lead to stale arduino_types.h or a stale Rust staticlib when those files change; add the missing sources to the dependency list (or depend on the whole src tree).
| set(RUST_ENUM_SRCS | |
| ${RUST_CRATE_DIR}/src/lib.rs | |
| ${RUST_CRATE_DIR}/src/string.rs | |
| ${RUST_CRATE_DIR}/src/arduino_types/common.rs | |
| ${RUST_CRATE_DIR}/src/arduino_types/hardware_spi.rs | |
| ${RUST_CRATE_DIR}/src/arduino_types/stream.rs | |
| ${RUST_CRATE_DIR}/src/arduino_types/ip_address.rs | |
| ${RUST_CRATE_DIR}/src/arduino_types/hardware_can.rs | |
| file(GLOB_RECURSE RUST_ENUM_SRCS | |
| ${RUST_CRATE_DIR}/src/*.rs |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 25 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return arduino_wchar_to_upper(c); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Missing newline at end of file. This violates POSIX standards and can cause issues with some tools.
| @@ -0,0 +1,14 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
Missing copyright and SPDX license header. All other new files in this PR have proper copyright headers with "Copyright (c) 2026 TOKITA Hiroshi" and "SPDX-License-Identifier: Apache-2.0".
| } | ||
|
|
||
| }; |
There was a problem hiding this comment.
Missing closing brace for the namespace. The file declares namespace arduino { on line 7 but never closes it with }. This will cause a compilation error.
| } | |
| }; | |
| }; | |
| } // namespace arduino |
| class HardwareI2C : virtual public Stream, virtual public idl::HardwareI2CIfc { | ||
| }; | ||
|
|
||
| }; |
There was a problem hiding this comment.
Missing newline at end of file. This violates POSIX standards and can cause issues with some tools.
| class Printable : virtual public idl::PrintableIfc { | ||
| }; | ||
|
|
||
| } |
There was a problem hiding this comment.
Missing newline at end of file. This violates POSIX standards and can cause issues with some tools.
| } | |
| } |
|
|
||
| char* ltoa(long value, char *string, int radix); //TODO | ||
| char* ultoa(unsigned long value, char *string, int radix); //TODO | ||
|
|
There was a problem hiding this comment.
Missing newline at end of file. This violates POSIX standards and can cause issues with some tools.
| class Server : virtual public Print, virtual public idl::ServerIfc { | ||
| }; |
There was a problem hiding this comment.
Missing semicolon after the closing brace of the class declaration. This will cause a compilation error.
| class Stream : virtual public Print, virtual public idl::StreamIfc { | ||
| }; |
There was a problem hiding this comment.
Missing semicolon after the closing brace of the class declaration. This will cause a compilation error.
| @@ -0,0 +1,43 @@ | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
Missing copyright header. Other proto files in this PR (e.g., print.proto, stream.proto) include a copyright notice with "Copyright (c) 2026 TOKITA Hiroshi" and SPDX-License-Identifier.
| return true; | ||
| } | ||
|
|
||
| let alloc_size = (needed_len as usize).saturating_add(1); |
There was a problem hiding this comment.
Potential integer overflow in buffer size calculation. If needed_len is close to c_uint::MAX, adding 1 could overflow. Consider checking if needed_len == c_uint::MAX before the addition to prevent undefined behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class Client : virtual public Stream, virtual public idl::ClientIfc { | ||
| protected: | ||
| uint8_t* rawIPAddress(IPAddress& addr); //TODO | ||
| }; |
There was a problem hiding this comment.
Missing semicolon after the closing brace of the class definition. Comparing to other files like Server.h (line 16), Stream.h (line 15), and Printable.h (line 18), all class definitions should end with a semicolon.
| rpc RequestFromStop(google.protobuf.Empty) returns (google.protobuf.Empty) { | ||
| option (arduino.cpp_decl) = "size_t requestFrom(uint8_t address, size_t len, bool stopBit)"; |
There was a problem hiding this comment.
Return type mismatch between proto definition and cpp_decl. The proto declares returns (google.protobuf.Empty) but the cpp_decl specifies size_t requestFrom(...) which returns a size_t, not void. This inconsistency could cause issues in code generation or runtime behavior.
| rpc RequestFrom(google.protobuf.Empty) returns (google.protobuf.Empty) { | ||
| option (arduino.cpp_decl) = "size_t requestFrom(uint8_t address, size_t len)"; |
There was a problem hiding this comment.
Return type mismatch between proto definition and cpp_decl. The proto declares returns (google.protobuf.Empty) but the cpp_decl specifies size_t requestFrom(...) which returns a size_t, not void. This inconsistency could cause issues in code generation or runtime behavior.
| return None; | ||
| } | ||
| out[0] = b'-' as c_char; | ||
| let magnitude = ((-(value + 1)) as u64) + 1; |
There was a problem hiding this comment.
Potential integer overflow in write_signed_base when computing magnitude for i64::MIN. The expression ((-(value + 1)) as u64) + 1 on line 554 will overflow when value is i64::MIN (-9223372036854775808). For i64::MIN, value + 1 is still negative, and negating it would cause overflow in two's complement arithmetic. A safer approach would be to use value.unsigned_abs() or handle i64::MIN as a special case.
| rpc BeginPacketHost(google.protobuf.Empty) returns (google.protobuf.Empty) { | ||
| option (arduino.cpp_decl) = "int beginPacket(const char *host, uint16_t port)"; |
There was a problem hiding this comment.
Return type mismatch between proto definition and cpp_decl. The proto declares returns (google.protobuf.Empty) but the cpp_decl specifies int beginPacket(...) which returns an int, not void. This inconsistency could cause issues in code generation or runtime behavior.
| rpc ReadUnsigned(google.protobuf.Empty) returns (google.protobuf.Empty) { | ||
| option (arduino.cpp_decl) = "int read(unsigned char *buffer, size_t len)"; |
There was a problem hiding this comment.
Return type mismatch between proto definition and cpp_decl. The proto declares returns (google.protobuf.Empty) but the cpp_decl specifies int read(...) which returns an int, not void. This inconsistency could cause issues in code generation or runtime behavior.
| rpc ReadChar(google.protobuf.Empty) returns (google.protobuf.Empty) { | ||
| option (arduino.cpp_decl) = "int read(char *buffer, size_t len)"; |
There was a problem hiding this comment.
Return type mismatch between proto definition and cpp_decl. The proto declares returns (google.protobuf.Empty) but the cpp_decl specifies int read(...) which returns an int, not void. This inconsistency could cause issues in code generation or runtime behavior.
| rpc RemoteIP(google.protobuf.Empty) returns (google.protobuf.Empty) { | ||
| option (arduino.cpp_decl) = "IPAddress remoteIP()"; |
There was a problem hiding this comment.
Return type mismatch between proto definition and cpp_decl. The proto declares returns (google.protobuf.Empty) but the cpp_decl specifies IPAddress remoteIP() which returns an IPAddress object, not void. This inconsistency could cause issues in code generation or runtime behavior.
| rpc Write(google.protobuf.Empty) returns (google.protobuf.Empty) { | ||
| option (arduino.cpp_decl) = "int write(CanMsg const &msg)"; |
There was a problem hiding this comment.
Return type mismatch between proto definition and cpp_decl. The proto declares returns (google.protobuf.Empty) but the cpp_decl specifies int write(...) which returns an int, not void. This inconsistency could cause issues in code generation or runtime behavior.
| rpc BeginPacketIp(google.protobuf.Empty) returns (google.protobuf.Empty) { | ||
| option (arduino.cpp_decl) = "int beginPacket(IPAddress ip, uint16_t port)"; |
There was a problem hiding this comment.
Return type mismatch between proto definition and cpp_decl. The proto declares returns (google.protobuf.Empty) but the cpp_decl specifies int beginPacket(...) which returns an int, not void. This inconsistency could cause issues in code generation or runtime behavior.
Headers
Implements