gauteh / defmt-serial

Log defmt messages over the serial port.
24 stars 7 forks source link

Added the ability to use blocking Write implementation. #5

Closed esden closed 11 months ago

esden commented 11 months ago

It is useful to also be able to use the blocking::Write trait implementation. Some libraries like embassy-rs only implement the blocking version of the Write trait but not the non blocking one.

gauteh commented 11 months ago

Do you think it would be possible for us to create an DefmtSerialTarget trait, implement it for whatever interface, and then just take that at the in defmt_serial? Then there would be only one entry point for the user.

gauteh commented 11 months ago

Actually it seems that most implementations of non-blocking Write would also have a default blocking write: https://docs.rs/embedded-hal/latest/src/embedded_hal/blocking/serial.rs.html#23 (but it is not guaranteed?). Since we need blocking write in defmt we should maybe only support blocking::Write. The default implementation is essentially the same as what we are doing anyway.

esden commented 11 months ago

I was about to get back to you and say we can implement a trait to provide both interfaces. But I think you are right that just defaulting to blocking is the way to go as it is what we need anyways. I can make the necessary change if you are ok with that.

esden commented 11 months ago

I went ahead and pushed the "just blocking" version of the patch. I hope that is ok. I will check tomorrow in case you change your mind and want me to do something different. :)

gauteh commented 11 months ago

Yes. I think this is the way to go. The only issue is that HAL's have to opt-in to the default implementation, by doing something like:

impl blocking::serial::write::Default for MySerial {}

When they are not, it's probably a bug in the HAL anyway (I think I might have this bug in ambiq-hal), but I guess users could bug the HAL and stick to the old version of defmt-serial in the meantime.

esden commented 11 months ago

I think that makes sense. At least there is a fallback. That said, take your time, maybe ask what the ambiq-hal folks think before you merge this. No rush I just use my branch as a [patch] for my code in the mean time. :)

gauteh commented 11 months ago

I added a bunch of CI tests. I also added the blocking Write impl to ambiq-hal (https://github.com/gauteh/ambiq-rs/blob/main/ambiq-hal/src/uart.rs#L136), but there is something not linking as it should. Maybe some outdated dependencies. Maybe you will figure it out before me.

esden commented 11 months ago

On my end the system is complaining that the Cargo.toml is referring to ambiq-rs v0.2 not v0.3. Updating that dependency did the trick over here.

gauteh commented 11 months ago

I forced push a rebase with updated deps to your PR. I am guessing the example pi pico, or at least the example-std needs to be adapted to your changes, while I have fixed the artemis example (through the HAL).

example-std may just need:

[dependencies]
critical-section = { version = "1.1", features = ["std"]}
esden commented 11 months ago

Ok, I worked out the changes needed for the example-std to compile.

Beyond that, I need to add rust-toolchain.toml in the toplevel and example projects to make things compile here as the project requires nightly toolchain. I don't have that set globally on my system. It might be a good idea to carry the rust-toolchain.toml files. But maybe there is a reason you don't have them and I rather ask before I add them to this PR.

gauteh commented 11 months ago

Thanks, merged. The nightly requirement is not introduced by us, and rust-toochain wont have any effect on dependents of the lib. So we'll leave that to the users of the library. Making a new release.

esden commented 11 months ago

Ok, sounds good. Thank you very much for merging and releasing! :)