reimplement log-logging feature

[fkohlgrueber]

This is a WIP PR to reintroduce the log-logging feature flag (as has been discussed in #86 ).

I first looked at the changes from #80 but then decided to introduce a new LossyStr type that's wrapping &[u8] and implementing formatting as a str (if possible) for both defmt and core::fmt::Debug. This way, there's no need to use feature flags when calling atat_log with a byte slice as param.

I haven't fully tested the code yet, but I wanted to open the PR to gather feedback.

Have a great day ;-)

[MathiasKoch]

but then decided to introduce a new LossyStr type that's wrapping &[u8] and implementing formatting as a str (if possible) for both defmt and core::fmt::Debug

That is an awesome way to do it! Can't believe i haven't though of that! 🧐

Looks good at first glance! 👍

[fkohlgrueber]

I'm currently thinking about how the CI scrips should be updated. I've implemented the features such that

1. no features -> no logging
2. just log-logging -> logging using log
3. any defmt-* features, but not log-logging -> logging using defmt
4. any defmt-* features and log-logging -> compile error

I think the CI should make sure that the code compiles for 1.-3.
I don't think we need to test that there's a compile error for 4.

Do you agree?

[MathiasKoch]

I think your condition checks might be simplified a bit by introducing a defmt-logging feature such that:

[features]
default = ["defmt-logging"]

defmt-logging = ["defmt"]
log-logging = ["log"]

defmt-default = ["defmt-logging"]
defmt-error = ["defmt-logging"]
defmt-warn = ["defmt-logging"]
defmt-debug = ["defmt-logging"]
defmt-info = ["defmt-logging"]

And then making the derive for defmt::Format conditional based on defmt-logging?

Furthermore it removes the need for defmt in your dependency graph, if you are only interested in using log?

What you think?

Regarding the CI i would agree with points 1, 2 & 3 being most relevant

[fkohlgrueber]

And then making the derive for defmt::Format conditional based on defmt-logging?

Furthermore it removes the need for defmt in your dependency graph, if you are only interested in using log?

That sounds really beneficial, especially because defmt can cause linker errors when compiled for x86.

Do we really need the defmt-logging and log-logging features? Simply using log and defmt should be enough, right? This would also be consistent to the way smoltcp does it, see smoltcp-rs/smoltcp#455.

[MathiasKoch]

I don't think a feature and an optional dependency can have the same name? 🤔

[dbrgn]

I don't think a feature and an optional dependency can have the same name? thinking

If the feature flag should simply enable/disable that one optional dependency, then it works. However, it cannot be used to "group" multiple optional dependencies.

[MathiasKoch]

Aha, TIL 😊👍

[dbrgn]

(Or, in other words: If you have an optional dependency on the crate "foobar", then "foobar" will automatically be a cargo feature, without even defining it. This means that you cannot define your own custom "foobar" flag, since it would conflict.)

[MathiasKoch]

Ahh that makes sense 👍

I just wasn't aware you could still #[cfg(feature = ..)] on optional dependencies.

[fkohlgrueber]

I updated the branch:

• defmt is now optional (feature "defmt" and implicitly used by "defmt-default" etc.)
• removed the trait bound E: defmt::Format from the Error enum (if the defmt feature isn't set, there's no way to enforce the trait bound. Only requiring it when the feature is used seems odd to me.)
• log-logging feature removed (simply use "log" feature)
• updated readme and CI script

I also tested the logging output on x86 (using log) and on a blue pill (using defmt):

# log
[2021-05-16T19:39:52Z DEBUG atat::client] Sending command: ""AT+CIPSTATUS\r\n""

# defmt
     723 DEBUG Sending command: "b"AT+CIPCLOSE\r\n""
└─ atat::client::{{impl}}::send @ /home/felix/tmp/atat/atat/src/helpers.rs:128

Seems to work fine. From my side, this looks ready to be integrated now. Can you review the PR please?

Thanks in advance!

[codecov]

Codecov Report

Merging #92 (a357168) into master (4d1c4f0) will increase coverage by 0.80%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage   41.59%   42.40%   +0.80%     
==========================================
  Files          25       25              
  Lines        1291     1257      -34     
  Branches      598      570      -28     
==========================================
- Hits          537      533       -4     
+ Misses        302      286      -16     
+ Partials      452      438      -14     

Impacted Files	Coverage Δ
atat/src/client.rs	32.78% <0.00%> (+1.53%)	⬆️
atat/src/error.rs	38.09% <ø> (+6.84%)	⬆️
atat/src/helpers.rs	73.91% <ø> (ø)
atat/src/lib.rs	57.14% <ø> (ø)
atat/src/traits.rs	58.10% <ø> (ø)
atat/src/ingress_manager.rs	42.50% <40.00%> (+6.94%)	⬆️
atat/src/digest.rs	47.14% <50.00%> (+2.69%)	⬆️
serde_at/src/de/mod.rs	45.50% <0.00%> (+0.94%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d1c4f0...a357168. Read the comment docs.

[MathiasKoch]

Looks good to me 👍