rust-ndarray
Made compatible with thumbv6m-none-eabi
Changes
Add the crates portable-atomic and portable-atomic-util to make the Arc used in the code compatible with the thumbv6m-none-eabi target. Also note that this change will probably make ndarray work on more targets than just the thumbv6m-none-eabi target.
Considerations
Those two crates are imported from a GitHub revision. It might be best to wait for an official release to crates.io, but I'm not sure if that's important.
This has also been tested and confirmed to work on a Raspberry Pi Pico.
Burn project's Ndarray backend would very much appreciate if this PR is merged. We have a long outstanding issue: https://github.com/tracel-ai/burn/issues/302
@bluss, do you have any more concerns about this pull request? If so, I would love to see if I can address them!
Clippy is fixed on current master. I just approved the CI run.
Not sure if I can review sensibly.
What issues make it so you might be unable to review sensibly? I can help with hardware if you need it. Additionally, I could write a test to check that it compiles correctly.
I did forget to mention, but here are the commands needed to compile for thumbv6m-none-eabi.
rustup target add thumbv6m-none-eabicargo build --target thumbv6m-none-eabi --no-default-features --features portable-atomic --features critical-section
@BjornTheProgrammer Main reason is I'm a quite bad maintainer, as you can see, so it takes some time. So always wary to take on more complexity especially in an area that is not something I know about.
@bluss thank you so much for the review! Don't worry about taking time, we are all busy 😅. I added the GitHub tests, but I don't have an environment to test that these are properly working, so a once-over would be much advised. Additionally I added a cfg flag to enable the dependency as suggested. I made an analysis of the user impact, and there honestly doesn't seem to be much, if any at all. However further testing should be done, just to verify that really is true.
Should I run these tests, or do you have any other suggestions/concerns?
I suggest copying this snippet from matrixmultiply and testing it like that. Just using stable, don't need any older version. (The "ensure_no_std/Cargo.toml" in this link also needs to be changed and not copied.)
https://github.com/bluss/matrixmultiply/blob/bb3dd0be9d8a9f592e567e4402950546bf710eba/.github/workflows/ci.yml#L94-L117
MSRV changed to 1.64 on master. Maybe that can solve some problems (but it wasn't changed because of this PR).
Best would be if you could squash together the changes into one commit. The merge queue preserves commits, but here we don't want to save the history. (The merge queue doesn't allow us to choose merge vs squash depending on the PR, that makes me think it's a big drawback of it.)
There I think I've fixed the history, just need to rename critical-section and squash, and then we should be good. What name would you like?
I've tested on actual hardware, and it seems to be working properly!
@bluss I would just like to thank you very much for the time and effort you put into this. Your contributions are extremely valued. Thanks once again!