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.

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

Use defmt or log #295

Merged
merged 5 commits into from
Aug 1, 2024
Merged

Conversation

WilliamTakeshi
Copy link
Contributor

This PR addresses issue #280 . It introduces two new optional dependencies: defmt and defmt-or-log. Additionally, it makes the log dependency optional.

The purpose of this change is to allow users to specify either log or defmt as a feature for the lib. This flexibility should simplify debugging the lib on microcontrollers.

As this is my first PR for this library, I am open to feedback and suggestions! Please let me know if any changes are needed.

@geonnave
Copy link
Collaborator

geonnave commented Jul 6, 2024

Thanks for this PR @WilliamTakeshi!

For my understanding:

  • are we still able to use RUST_LOG to set the log level?
  • since both log and defmt are optional, should I select one via features or there is one that is the default? (maybe log is the default since the CI just passed?)

Before merging, I would like to have it tested on an embedded device, like running the no_std example in the nRF52840.
However, I am leaving for holidays next week and I don't have a board at home, so I will tag @malishav / @chrysn: if one of you guys could verify that it just works as before.

@WilliamTakeshi
Copy link
Contributor Author

are we still able to use RUST_LOG to set the log level?

Yes, RUST_LOG is still supported (I have tested it using the log feature). For example, you can test it with the following commands:

RUST_LOG=error cargo run --bin coapclient
RUST_LOG=trace cargo run --bin coapserver

allowing you to see only logs on the server.

since both log and defmt are optional, should I select one via features or there is one that is the default? (maybe log is the default since the CI just passed?)

By default, neither log nor defmt is enabled. You can enable either one by specifying the desired feature. As for the CI passing, I wasn't aware that we had a test for the log, so it might have been luck that it passed!

In addition to specifying one of these features, you could also pass another logger configuration, such as embassy-sync/defmt or embassy-sync/log. However, it would be great if someone with an embedded device could test this to confirm.

@malishav
Copy link
Contributor

I can confirm I successfully tested the native examples with:

RUST_LOG=error cargo run --bin coapclient
RUST_LOG=trace cargo run --bin coapserver

That is OK.

I am not sure what exactly to test on nrf52840dk. I tried adding defmt-or-log and defmt as dependencies in lakers-no_std example which runs on nrf52840, and adding an info line in the source code. However, when I run this with cargo embed I get the println! prints in the output, but not the info line. I also tried setting the log level via RUST_LOG while running cargo embed but to no avail.

As a summary, it would be great if the PR included the changes for the lakers-no_std example and instructions how to test.

@WilliamTakeshi
Copy link
Contributor Author

@malishav Thanks for testing!

Unfortunately, I don't have an embedded device to test this on, making it harder for me to verify the implementation fully. Have you tried adding the features when importing lakers? For example:

for example: lakers = { package = "lakers", path = "../../lib", features = [ "defmt" ] } I also know that we can use others like:

  • "defmt-or-log/defmt"
  • "heapless/defmt-03"
  • "embassy-sync/defmt"
  • "embedded-hal-async/defmt-03"

depending on which embedded device are you using.

On a side note, do you guys recommend any cheap device I can use to test it?

@malishav
Copy link
Contributor

Have you tried adding the features when importing lakers? For example:

for example: lakers = { package = "lakers", path = "../../lib", features = [ "defmt" ] } I also know that we can use others like:

  • ``

Yes, tried that, without it wouldn't build. It seems using defmt requires a special linker script, which is probably what is missing.

For embedded, our go-to hardware is the nRF52840 chip for now and we are extensively using nRF52840-DK development board.

@WilliamTakeshi
Copy link
Contributor Author

Sorry for the late reply!
I bought a nRF52840-DK but it took a while to ship it to me.

@malishav, thank you for adding examples for the nRF52840! Using these examples, I was able to get defmt_or_log to work. Since I only have one board, I wasn't able to run the entire test, but I did start the initiator with the following command:

PROBE_RS_PROBE=<PROBE_NUMBER> DEFMT_LOG=trace cargo run --bin initiator

Here is the output I received:

INFO  Starting BLE radio
└─ initiator::____embassy_main_task::{async_fn#0} @ src/bin/initiator.rs:44  
INFO  init_handshake
└─ initiator::____embassy_main_task::{async_fn#0} @ src/bin/initiator.rs:59  
TRACE Initializing EdhocInitiator
└─ lakers::{impl#5}::new @ /home/william/projects/rust/lakers/lib/src/lib.rs:251 
TRACE Enter prepare_message_1
└─ lakers::{impl#5}::prepare_message_1 @ /home/william/projects/rust/lakers/lib/src/lib.rs:273 
TRACE Enter parse_message_2
└─ lakers::{impl#6}::parse_message_2 @ /home/william/projects/rust/lakers/lib/src/lib.rs:313

@geonnave
Copy link
Collaborator

Thanks for the update @WilliamTakeshi, the output seems like what it should be, with the trace logs from the library showing up.
I wasn't able to make it work with logs in the no_std example, but that is using cargo embed which is deprecated, so I hope that once we update it to probe-rs the error goes away.

Can you add the following to the README before we merge?

## Example: using logs
Logs can be used in both native and embedded applications. Once configured in an application, both can be controlled via environment variables:

- on native, set `RUST_LOG` to control Rust's built-in `log` facility
- on embedded, set `DEFMT_LOG` to control the [defmt](https://github.com/knurling-rs/defmt) crate

The selection of `log` or `defmt` is handled internally by the [defmt-or-log](https://github.com/t-moe/defmt-or-log) crate.

For example, `examples/lakers-nrf52840` is configured to use `defmt`. To globally enable logs at level `trace`:
```bash
DEFMT_LOG=trace cargo run --bin initiator
```

Different log levels can also be set per crate or module.
Here's how to globally set logs to level `trace`, while restricting `lakers` to level `info`:
```bash
DEFMT_LOG=trace,lakers=info cargo run --bin initiator
```

@WilliamTakeshi
Copy link
Contributor Author

I also couldn't get examples/lakers-no_std to work. If needed, I can try updating it to use probe-rs.

By the way, should the new paragraph be added to the root README.txt or the examples/lakers-nrf52840/README.txt? I added on the root

@geonnave
Copy link
Collaborator

geonnave commented Aug 1, 2024

I can try updating it to use probe-rs

That would be very welcome! Note that it's already tracked in #298.

And yes I think the right place is in the root README.

@geonnave geonnave merged commit ca4b84c into openwsn-berkeley:main Aug 1, 2024
31 checks passed
@geonnave geonnave mentioned this pull request Aug 2, 2024
@geonnave geonnave added the enhancement New feature or request label Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants