Closed D1plo1d closed 2 years ago
@DrLuke I think this latest commit should resolve both the issues you raised however feel free to un-resolve any threads if they need more discussion.
I've also confirmed the code is working in my embedded environment (ie. alloc / nostd) so hopefully this should be good to go!
Hey 🖖🏻
I'm very happy to see that PR and will hopefully review it on Sunday, together with #20 . Also, I'm sorry for the big delay, was quite busy recently (to my defense).
PS: Thanks @DrLuke for having a first look on it.
I just merged #20 , can you please rebase onto master
?
I just merged #20 , can you please rebase onto
master
?
Done!
Just a note on the rebase the address matcher doesn't support no_std
due to it's reliance on the regex
crate. In addition to lacking no_std
the regex
crate can as I understand it also be quite a large dependency.
It doesn't look like our regex
usage is too extensive atm so it might be worth reworking the pattern matching logic in address to be entirely nom-based in future.
It doesn't look like our
regex
usage is too extensive atm so it might be worth reworking the pattern matching logic in address to be entirely nom-based in future.
Pattern matching as defined in the OSC spec is actually a subset of regular expressions. I also tried to implement it in the first run without relying on the regex
crate, but it was just convenient to translate the patterns into regular expressions and offload the heavy work this way.
I do not want to keep you busy with this PR any longer and so my suggestion is to merge this as-is without building a release and opening an issue to track the replacement of regex with some custom nom
logic[^1]. What do you think of that?
[^1]: Of course something I could do then, or some friendly contributor 😸
That sounds perfect. Leaving no_std OSC patterns as a known opportunity for future improvement sounds exactly the 80:20 balance point for me.
I might then suggest a release if no one jumps on the no_std patterns ticket a certain time after the merge. Introducing a breaking change later to add no_std patterns with a 0.X minor version might be IMO better then having consumers of the crate depend on the git version for too long.
Sounds fine to me, let's go forward as suggested and merge this. Thanks again for the contribution @D1plo1d 👍🏻
I've ported rosc over to nom in order to enable no-std support for use in embedded and WASM environments. Initially this was just for my own use on a microcontroller which receives OSC commands but I saw @klingtnet 's mention here:
https://github.com/klingtnet/rosc/commit/62fa9e258bd3f9fbfe0a592842b453df30e42328
So I figured I should create a pull request proper - at the very least to have a thread for further discussion.
This is going back a bit for me (I context switched to a completely unrelated work topic last week so please excuse the hazy memory) but as I remember it I think I'd gotten it working well and compiling without issue in my esp32 no-std environment.
There were some public API changes so this would necessitate a version bump from
0.5
to0.6
.