Skip to content
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.

Sign up for GitHub

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

Jump to bottom

pkg/wamr: add WAMR to provide WASM support in RIOT #15329

Merged
merged 13 commits into from
Mar 2, 2022
Merged

pkg/wamr: add WAMR to provide WASM support in RIOT #15329

merged 13 commits into from
Mar 2, 2022

Conversation

kfessel
Copy link
Contributor

@kfessel kfessel commented Oct 28, 2020 •
edited
Loading

Contribution description

This PR adds WAMR as a pkg to RIOT to enable Webassembly execution on RIOT

tested on native and nucleo-f767zi

RIOT compatibility is upsteamed

Testing procedure

Example will be provided

references

https://github.com/bytecodealliance/wasm-micro-runtime

bytecodealliance/wasm-micro-runtime#357 < about wasm-memory

@miri64 miri64 added Area: pkg Area: External package ports Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Oct 28, 2020
@chrysn
Copy link
Member

chrysn commented Oct 30, 2020

Looking forward to trying this out.

fjmolinas
fjmolinas reviewed Nov 26, 2020
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @kfessel, some comments on the makefile integration. Regarding
the PR description:

Example will be provided

Would you say this PR is still WIP then?

pkg/wamr/Makefile Outdated Show resolved Hide resolved
pkg/wamr/Makefile Outdated Show resolved Hide resolved
pkg/wamr/Makefile Show resolved Hide resolved
pkg/wamr/Makefile Outdated Show resolved Hide resolved
pkg/wamr/Makefile Outdated Show resolved Hide resolved
pkg/wamr/Makefile Outdated Show resolved Hide resolved
pkg/wamr/CMakeLists.txt Show resolved Hide resolved
@fjmolinas
Copy link
Contributor

Example will be provided

Would you say this PR is still WIP then?

Asking because we need some compile test for merging.

@kfessel
Copy link
Contributor Author

kfessel commented Nov 26, 2020 •
edited
Loading

yes it is WIP

I got a RIOT example that is upstreamed to WAMR that to get RIOT as a platform into WAMR should i add that (it is very basic)?

@kfessel
Copy link
Contributor Author

kfessel commented Nov 26, 2020 •
edited
Loading

I got a kernel_type.h include upstream and will try to fix it there. I don't know: if there was a deprecation warning, but i certainly did not see it.
upstreaming
bytecodealliance/wasm-micro-runtime#452

@kfessel
Copy link
Contributor Author

kfessel commented Nov 26, 2020

The example I just uploaded is the one i provided to the WAMR upstream just with the makefile turned around a little.

@kfessel
Copy link
Contributor Author

kfessel commented Nov 27, 2020

The warning kernel_types.h is removed (upstream patched)

some how the WPEDANTIC := 0 in the Makefile is ignored (there are warnings About void* to function pointer casts) if the alocator functions are defined.

@fjmolinas
Copy link
Contributor

yes it is WIP

I got a RIOT example that is upstreamed to WAMR that to get RIOT as a platform into WAMR should i add that (it is very basic)?

@kfessel let me know when this is no longer WIP and I'll take another look :)

@kfessel kfessel changed the title pkg/wamr: add WAMR to provide WASM support in RIOT WIP: pkg/wamr: add WAMR to provide WASM support in RIOT Dec 11, 2020
@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jun 16, 2021
@stale stale bot closed this Jul 20, 2021
@aabadie
Copy link
Contributor

aabadie commented Jul 20, 2021

ping @kfessel, is this still WIP ? Can it be reopened ?

@kfessel
Copy link
Contributor Author

kfessel commented Jul 21, 2021 •
edited
Loading

yes it is

This has the interpreter running but missing calls to system (Riot) making it more useful than to say i an able to run wasm on riot. i am currently trying to generalise system calls for Riot embedded interpreters since i wasnt able to find something i could tap into

@kfessel
Copy link
Contributor Author

kfessel commented Sep 3, 2021

I like to have this open

@kfessel kfessel reopened this Sep 3, 2021
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Sep 3, 2021
@github-actions github-actions bot added Area: doc Area: Documentation Area: examples Area: Example Applications labels Sep 3, 2021
@kfessel kfessel added State: don't stale State: Tell state-bot to ignore this issue State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Sep 3, 2021
@kfessel kfessel force-pushed the p-add-wamr branch 2 times, most recently from 40f5087 to fb5791e Compare September 8, 2021 13:12
maribu
maribu reviewed Sep 22, 2021
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of style nits

examples/wasm/iwasmt.c Outdated Show resolved Hide resolved
examples/wasm/iwasmt.c Outdated Show resolved Hide resolved
examples/wasm/iwasmt.c Outdated Show resolved Hide resolved
examples/wasm/iwasmt.c Outdated Show resolved Hide resolved
examples/wasm/iwasmt.c Outdated Show resolved Hide resolved
examples/wasm/iwasmt.c Outdated Show resolved Hide resolved
examples/wasm/iwasmt.c Outdated Show resolved Hide resolved
pkg/wamr/Makefile Outdated Show resolved Hide resolved
pkg/wamr/Makefile.dep Outdated Show resolved Hide resolved
pkg/wamr/Makefile.include Outdated Show resolved Hide resolved
@maribu
Copy link
Member

maribu commented Sep 22, 2021

Would it be possible that you extend the example so that one is able to recreate the main.wasm? E.g. by placing the source (including a Makefile or similar and a README.md) into a subfolder named e.g. wasm-source?

I am aware that generating WASM bytecode isn't really specific to RIOT, but fine-tuning e.g. the stack size, pointing out that WAMR contains a subset of the standard C lib as built ins (e.g. puts()) etc. is still non-obvious.

@kfessel kfessel removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: full build disable CI build filter labels Feb 11, 2022
@bergzand
Copy link
Member

I played around a bit with the code here. In general the experience is pretty good and it was easy enough to get things to work and add bindings for calls (saul) to the code. I haven't really tried passing arguments to the code or copy structs to the virtual memory of the runtime. I asked offline for the clarification provided in fc1f44a as my first action was to copy the bits from the iwasm.c file.

@kfessel kfessel requested a review from maribu February 23, 2022 13:46
@emmanuelsearch
Copy link
Member

@kfessel @maribu so does it seems close to merging this into master?

@maribu
Copy link
Member

maribu commented Mar 2, 2022 •
edited by kfessel
Loading

@kfessel: Please check:

@kfessel
example/wasm: remove allocator selection
3400819 
add comment refering to the wasm header instead
@maribu
Copy link
Member

maribu commented Mar 2, 2022

Please squash! IMO two commits (one for the pkg, one for the example) are sufficient.

@benpicco
Copy link
Contributor

benpicco commented Mar 2, 2022

Yea two commits (one for the pkt, one for the pkg) are fine, we don't need the entire work history in tree.
But then again this is purely bikeshedding, I'm not that concerned about git log as long as everything is properly prefixed.

@kfessel
Copy link
Contributor Author

kfessel commented Mar 2, 2022

Yea two commits (one for the pkt, one for the pkg) are fine, we don't need the entire work history in tree. But then again this is purely bikeshedding, I'm not that concerned about git log as long as everything is properly prefixed.

everything is prefixed, git log will just show the one-merge commit if it is not fast forwarded.

and murdock is done 🟩

@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Mar 2, 2022
maribu
maribu approved these changes Mar 2, 2022
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Code looks good and works like a charm. Let's not have this delayed over bike shedding regarding commit history.

@maribu maribu enabled auto-merge March 2, 2022 18:58
@maribu maribu merged commit a5b9136 into RIOT-OS:master Mar 2, 2022
@fjmolinas
Copy link
Contributor

Congrats @kfessel!

@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjmolinas fjmolinas fjmolinas left review comments

@maribu maribu maribu approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@bergzand bergzand Awaiting requested review from bergzand

Assignees
No one assigned
Labels
Area: doc Area: Documentation Area: examples Area: Example Applications Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines State: don't stale State: Tell state-bot to ignore this issue Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2022.04
Development

Successfully merging this pull request may close these issues.