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

Update defmt_decoder #2167

Merged
merged 1 commit into from
Feb 10, 2024
Merged

Update defmt_decoder #2167

merged 1 commit into from
Feb 10, 2024

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Feb 10, 2024

Closes #2153
Closes #2155

knurling/ferrous really doesn't want us using defmt_decoder, so I'm open to alternatives

@bugadani bugadani added dependencies Pull requests that update a dependency file skip-changelog This PR does not require a changelog entry on release labels Feb 10, 2024
Copy link
Contributor

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@MabezDev MabezDev added this pull request to the merge queue Feb 10, 2024
Merged via the queue into probe-rs:master with commit a003183 Feb 10, 2024
19 of 20 checks passed
@bugadani bugadani deleted the decoder branch February 10, 2024 15:51
t-moe pushed a commit that referenced this pull request Feb 20, 2024
@Urhengulas
Copy link
Contributor

knurling/ferrous really doesn't want us using defmt_decoder, so I'm open to alternatives

We do! What would make it easier for you?

@bugadani
Copy link
Contributor Author

We do! What would make it easier for you?

That's great! However, the library is currently marked as follows:

//! This is an implementation detail of [`probe-run`](https://github.com/knurling-rs/probe-run) and
//! not meant to be consumed by other tools at the moment so all the API is unstable.

In my opinion this is not a very friendly guarantee (or, lack thereof), especially when the internals are tied to an archived/deprecated project :)

From my personal POV, defmt has been around unstably for long enough without much change and probably without huge need for change. In my opinion it's time to start moving down the 1.0 route, or at least to offer some better-then-nothing guarantees for incorporating crates into third party host software.

All I'd like to see is probe-rs to not break if a new defmt_decoder is released. This issue also affects espflash, which is similarly a consumer of defmt_decoder (and defmt_parser for some reason I no longer remember). I understand that semver doesn't guarantee any stability for 0.x releases. I also understand that certain other aspects (like MSRV changes!) are not covered by semver at all. However, cargo's version resolution stil considers patch releases compatible, even for 0.x. Most libraries I've interacted with usually consider this compatibility when they introduce breaking changes.

Since we now have the library pinned to =0.3.10 maybe possible breaking changes are no longer an issue, but now we need to spend time evaluating whether we can or can not update to a new patch release with reasonable effort.

Sorry if this got a bit rant-y.

@Urhengulas
Copy link
Contributor

Sorry if this got a bit rant-y.

No worries. I get the point.

From my personal POV, defmt has been around unstably for long enough without much change and probably without huge need for change. In my opinion it's time to start moving down the 1.0 route, or at least to offer some better-then-nothing guarantees for incorporating crates into third party host software.

To be honest it was not a very conscious decision to keep the API unstable. It was more of a case of "it works for us", so we didn't bother stabilizing it.

And a small nit: the defmt API is stable. What is unstable are the "internal" crates defmt-parser, defmt-decoder etc. But I agree that it has been around for long enough to be stabilized.

I will raise the topic internally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file skip-changelog This PR does not require a changelog entry on release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't build with defmt-decoder 0.3.10
3 participants