rust: Add a Rust compatible API implementation. #141
Conversation
soburi
changed the title
soburi
force-pushed
the
rust_impl
branch
6 times, most recently
from
7a556b5 to
f86a047
Compare
soburi
changed the title
soburi
requested review from
DhruvaG2000,
beriberikix and
szczys
as code owners
pillo79
left a comment
pillo79
left a comment
There was a problem hiding this comment.
Awesome, looking forward to see this grow! 🥳 🤗
Kconfig
Outdated
| imply CBPRINTF_FP_SUPPORT | ||
| imply RING_BUFFER | ||
| select UART_INTERRUPT_DRIVEN | ||
| select RUST |
There was a problem hiding this comment.
I think this should currently be an optional feature, to avoid adding the strong dependency right away.
| run: | | ||
| git clone https://github.com/arduino/ArduinoCore-API.git $MODULE_PATH/../ArduinoCore-API | ||
| cp -rfp $MODULE_PATH/../ArduinoCore-API/api $MODULE_PATH/cores/arduino/ | ||
| apt install rustup |
There was a problem hiding this comment.
The recommended way to install Rust is via the rustup.rs installer script, Ubuntu's rustup package from apt is often outdated and may not work correctly
There was a problem hiding this comment.
Also,
All initialization is lumped into one "Initialize" step mixing unrelated concerns (cloning API, copying files, Rust setup)...
Better structure:
- name: Clone ArduinoCore-API
[...]
- name: Install Rust toolchain
[...]
There was a problem hiding this comment.
Fixed. Thank you for your comment.
.github/workflows/build.yml
Outdated
| rustup target add thumbv6m-none-eabi | ||
| rustup target add thumbv7em-none-eabihf | ||
| rustup target add thumbv7em-none-eabi | ||
| rustup target add thumbv7m-none-eabi |
There was a problem hiding this comment.
Installing targets one-by-one is slower. Rustup supports installing multiple targets in one command
.github/workflows/build.yml
Outdated
| rustup target add thumbv6m-none-eabi | ||
| rustup target add thumbv7em-none-eabihf | ||
| rustup target add thumbv7em-none-eabi | ||
| rustup target add thumbv7m-none-eabi |
There was a problem hiding this comment.
| rustup target add thumbv7m-none-eabi | |
| rustup target add thumbv6m-none-eabi thumbv7em-none-eabihf thumbv7em-none-eabi thumbv7m-none-eabi |
| let num = x.wrapping_sub(in_min).wrapping_mul(out_max.wrapping_sub(out_min)); | ||
| let den = in_max.wrapping_sub(in_min); | ||
| // Note: To keep compatibility, the panic when den=0 is left as is. | ||
| num / den.wrapping_add(out_min) |
There was a problem hiding this comment.
Due to operator precedence, this evaluates as:
num / (den + out_min) // Division happens AFTER adding out_min to den!
Original C++ code:
return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;Can we just follow similar arithmetic syntax in rust too, the code using wrapping_* is making the logic really confusing to understand, and with rust already being a new addition I feel like we should make an effort to keep things simple where possible.
There was a problem hiding this comment.
Since this is not a "new feature" but a reimplementation of the original implementation, we believe compatibility should be maintained.
From another perspective, this implementation is an internal implementation of the C++ API,
and therefore does not take advantage of Rust's features.
To take advantage of Rust's features would require a complete overhaul of the API set, so we think it's better to leave it this way for now.
It would also be good to have an option for more Rust-like behavior in the future.
Kconfig
Outdated
| config USE_ARDUINO_API_RUST_IMPLEMENTATION | ||
| bool "Use Rust implementation API core" | ||
| default y |
There was a problem hiding this comment.
Sorry to be nitpicky, but I asked this before and I'm still confused. 🙂
Is Rust expected to be installed by anyone trying to use the core at this stage?
If not, I would expect this to be manually set somehow (via the west command line, using snippets, etc) and not being default y-ed.
There was a problem hiding this comment.
+1 @pillo79
I would prefer this be optional and not default y too.
There was a problem hiding this comment.
OK. This is still in an experimental state.
Removed the default y.
We will gradually add a Rust compatible API implementation conforming to Apache-2. This PR adds an alternative implementation of Common.cpp. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Adding Rust architecture modules for ensure compilation. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
3 participants
We will gradually add a Rust compatible API implementation conforming to Apache-2.
This PR adds an alternative implementation of Common.cpp.