Skip to content

Allow to disable logging#23

Merged
flosse merged 2 commits intoslowtec:masterfrom
Abestanis:feature/log_feature
Sep 21, 2025
Merged

Allow to disable logging#23
flosse merged 2 commits intoslowtec:masterfrom
Abestanis:feature/log_feature

Conversation

@Abestanis
Copy link
Contributor

An application may want to do it's own logging using the returned errors.

@Abestanis
Copy link
Contributor Author

Great timing, since the merge of #21 yesterday, the cargo-all-features command managed to release an update that apparently changes their command line interface in a non-backwards compatible way. Hopefully they don't make a habit out of it.

@Abestanis
Copy link
Contributor Author

@flosse I see that 2b8e6cc basically reverted the CI changes of #21, was that intensional?

The benefit of cargo-all-features is that it runs all permutations of the features, not just one run with all features (besides the name), which I think is beneficial and can help catch errors such as the compile error fixed by #21.

If that is still something that you think would be good to have, commit aa12095 on this branch fixes the usage of cargo-all-features after their update.

@flosse
Copy link
Member

flosse commented Sep 20, 2025

I still don't understand why the log crate has to be disabled. It's just an API that is only used if you also have a logger.

And if you want something of your own, you just need to implement it: https://docs.rs/log/latest/log/#implementing-a-logger

@flosse
Copy link
Member

flosse commented Sep 20, 2025

@flosse I see that 2b8e6cc basically reverted the CI changes of #21, was that intensional?

This project introduced a breaking change in a version change from v1.10 -> v.1.11 without any documentation or CHANGELOG or similar. Therefore, I am not particularly interested in spending time on what has changed now and fixing the problems.

@Abestanis
Copy link
Contributor Author

I still don't understand why the log crate has to be disabled. It's just an API that is only used if you also have a logger.

In my application I do have a logger and I do have logs of my own, but I'm not interested by the logs that are emitted by this crate and would rather disable them. After all, I want to be able to controll what shows up in the log.

And if you want something of your own, you just need to implement it: https://docs.rs/log/latest/log/#implementing-a-logger

This is not about implementing my own logging, just the abillity to disable the logs created by this crate.

This project introduced a breaking change in a version change from v1.10 -> v.1.11 without any documentation or CHANGELOG or similar.

I hear you, a breaking change like that, especially on a semver stable version change, is really not a good sign. I am hoping that they will get at least some negative feedback and learn their lesson to not do it again. We could also pin the version to prevent any possible future breaking update like that.

Therefore, I am not particularly interested in spending time on what has changed now and fixing the problems.

In terms of spending time, no extra time would be required since I already found the necessary changes in aa12095. In my opinion it is still worth using this create, since manually creating CI jobs for all feature permutations is both annoying to maintain and easy to mess up.

If you still don't want to use that crate, I understand. Please let me know and I will remove the relevant commit.

@flosse
Copy link
Member

flosse commented Sep 20, 2025

In terms of spending time, no extra time would be required since I already found the necessary changes in aa12095. In my opinion it is still worth using this create, since manually creating CI jobs for all feature permutations is both annoying to maintain and easy to mess up.

I see. I took your commit and tried to adjust it in #29 but I wanted to get rid of the deprecated actions-rs And to be honest, I've never really looked into GitHub Actions, so something still isn't working 🙈 So if you have an idea how we can get this to work with maintained things, I agree to continue using it.

@flosse
Copy link
Member

flosse commented Sep 20, 2025

In my application I do have a logger and I do have logs of my own, but I'm not interested by the logs that are emitted by this crate and would rather disable them

Sure, but why do you need to use a feature flag for this?
You don't lose anything if you don't use it, or am I misunderstanding something?

From https://docs.rs/log/latest/log/:

If no logging implementation is selected, the facade falls back to a “noop” implementation that ignores all log messages. The overhead in this case is very small - just an integer load, comparison and jump.

so by not using the facade you implicitly disable it anyway.

After all, I want to be able to controll what shows up in the log.

Don't get me wrong, I don't want to slow you down, but do you have a specific example where you are currently unable to write your own custom logs?

@Abestanis
Copy link
Contributor Author

You don't lose anything if you don't use it, or am I misunderstanding something?

Yeah, there is a missunderstanding somewhere. Let me try to clarify:

I do want to use the log crate in my application for logging, it is very usefull. So I do have a working logger implementation and I'm not using the "noop" implementation. You can only select one implementaton per program, it is not possible to select one per crate. If I would select the "noop" implementation, I would not be able to log at all.

The log crate does not allow deactivating logs per crate either. You can filter by level, but that is too coarse and since these are warn and error logs, deactivating these would block all of my usefull warnings and errors as well.

So in short, I think the missunderstanding is that there is no easy way of diabling the logging from this crate that allows to use the log crate for my own logging.

do you have a specific example where you are currently unable to write your own custom logs?

My specific example is a program based on the embassy cratesrunning on a microcontroller that uses UART for logging, similat to how the embassy-usb-logger crate does it. The program also does some modbus communcation using this crate and an Ethernet adapter. My code looks roughly like this:

// [...]
log::info!("Some usefull log message");
// [...]
loop {
  // [...]
  match modbus_core::codec::tcp::client::decode_response(buffer) {
    Ok(response) => handle_response(response),
    Err(error) => if some_condition {
      // do something else
    } else {
      log::warn!("My own error message: {error:?}");
    }
  }
}

but why do you need to use a feature flag for this?

We could somehow add a parameter to controll this, but in my case I never want any log except my own. Adding a parameter for that also feels very clunky, since there are no default arguments in Rust. On top of that, even if it is not the case for my current project, some microcontrollers have very limited flash space available so every byte counts. Being able to remove dependencies and unwanted code via a feature can be really helpfull in those cases.

@flosse
Copy link
Member

flosse commented Sep 21, 2025

All right, now I understand. Thank you for your patience and detailed explanation 🙏

@flosse flosse merged commit 9162a12 into slowtec:master Sep 21, 2025
4 checks passed
@Abestanis Abestanis deleted the feature/log_feature branch September 21, 2025 13:03
AsgerWenneb pushed a commit to AsgerWenneb/modbus-core that referenced this pull request Dec 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants