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

Debug ergonomics #280

Open
chrysn opened this issue May 23, 2024 · 5 comments
Open

Debug ergonomics #280

chrysn opened this issue May 23, 2024 · 5 comments

Comments

@chrysn
Copy link
Collaborator

chrysn commented May 23, 2024

Currently, our errors are coarse-grained (which is fine), and are returned as results rather than panicking (which is also fine).

A consequence of this is that it is very hard to trace down where an error occurs, because you can't just set break points to the constructor of (say) an EDHOCError::ParsingError, so without a time-travelling debugger and without std printf, it is very hard to find where parsing actually failed. (For example, to find #279 (comment), I added a ParsingErrorS(&'static str) variant and changed all potential construction sites to include a label).

Thing is, I don't know a good way to do that so that we can have the labels around. The labels I'd like to have are not intended as content for the error messages, merely as debug output (winding up in the impl Debug of the errors), and on production builds, those should be elided at compile time. None of the options seem to work right:

  1. If we add a ('static str) content to all the error variants, we'll have to add a label to all constructors (much work but OK), but we can't compile time disable it with a #[cfg()] unless we also cfg-gate the arguments in the constructors (tedious)
  2. If we change EDHOCError to be a struct { EDHOCErrorVariant, Option<'static str> }, we'll have to construct them through the internal variant as EDHOCErrorVariant::ParsingError.with_label("foo"), need to add a .without_label() or .into() whenever we directly return errors (on ? paths we can skip it and use the automatic .into()), and the match branches grow more verbose.
  3. We could simplify 2 a bit if the EDHOCError had associated constants EDHOCError::ParsingError = EDHOCError(EDHOCErrorVariant::ParsingError, None), but that requires duplicating the variants across both types, requires a clippy override to allow an associated constant to use enum variant naming conventions, and will be super confusing in match because you can list all the variants and then the compiler complains about unhandled variants.

So, the issue is a bit annoying (esp. during plug testing), but I like none of the solutions I can come up with. Do you have better ones?

@geonnave
Copy link
Collaborator

geonnave commented May 23, 2024

Yeah, normally I only do "hot debug", i.e. rust-gdb --tui target/debug/coapclient, but that needs to be interactive.

I think a simple and good enough solution would be a logging system, but of course we don't have stdout so, I also don't have a solution.

... or maybe we could have it just be enabled for native builds, and have it be a no-op for embedded ones; that would be of some help.
I don't recall though if it is easy to do, maybe something like #![cfg_attr(not(feature = "python-bindings"), no_std)] but for the logging definitions.

@chrysn
Copy link
Collaborator Author

chrysn commented May 23, 2024

Logging might be a good idea: We'll need to check whether pulling in the log crate is really as free as I think it is (when logging is not enabled), and whether hax successfully ignores it when disabled, but at worst we'd have a trace!() or error!() macro that by default does nothing but if enabled calls the log macros. Maybe we can find an easy idiom to wrap that around some errors at creation time.

@geonnave
Copy link
Collaborator

Btw, just found out about defmt (and defmt-or-log), might be useful.

@chrysn
Copy link
Collaborator Author

chrysn commented May 23, 2024

defmt is awesome. I was unaware of defmt-or-log, that even more so.

@geonnave
Copy link
Collaborator

geonnave commented Aug 2, 2024

After #283 and #295, would you consider this issue solved, @chrysn ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants