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.

Sign up for GitHub

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

Jump to bottom

derive(Format): Support more than 256 variants #302

Merged
merged 1 commit into from
Dec 16, 2020
Merged

derive(Format): Support more than 256 variants #302

merged 1 commit into from
Dec 16, 2020

Conversation

Sh3Rm4n
Copy link
Contributor

@Sh3Rm4n Sh3Rm4n commented Dec 15, 2020

Closes #63

japaric
japaric reviewed Dec 15, 2020
View reviewed changes
Copy link
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

The encoder part looks good but I think we also need to do changes in the decoder.

Could you add the EnumLarge as a test to firmware/log.rs and test out a small enum and a large (>256) enum?

japaric
japaric reviewed Dec 15, 2020
View reviewed changes
firmware/qemu/src/bin/log.rs Outdated
}

defmt::info!("Format EnumLarge Variant: {:?}", EnumLarge::A007);
defmt::info!("Format EnumLarge Variant: {:?}", EnumLarge::A269);
Copy link
Member

Choose a reason for hiding this comment

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

this fails locally with

0.000101 INFO Format EnumLarge Variant: A007
failed to decode defmt data: [0, 99, 1]
Error: malformed data

I think the decoder expects that the discriminant is encoded as a single byte but with these changes it should consider how many the variants the enum has (and use 1,2,3,4,etc. bytes as the discriminant size)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops interesting. It's failing locally for me as well. I just pushed it to have a failing case to move along and eventually make it pass.

But that this does not fail in CI is interesting.

Copy link
Member

@japaric japaric Dec 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think CI is not running because the PR has merge conflicts. The last CI run (I'm referring to the GitHub Action runs on this repo; if you are referring to the ones on you fork, I've not seen those) is from your very first version, which didn't have the qemu test.

@Sh3Rm4n Sh3Rm4n force-pushed the enum branch 2 times, most recently from ff704da to 2bacbd9 Compare December 15, 2020 22:24
@japaric
Copy link
Member

japaric commented Dec 16, 2020

Nice! Thanks for the PR.

bors r+

bors bot added a commit that referenced this pull request Dec 16, 2020
@bors @Sh3Rm4n
Merge #302
1f67b84 
302: derive(Format): Support more than 256 variants r=japaric a=Sh3Rm4n

Closes #63

Co-authored-by: Sh3Rm4n <f.vioel@googlemail.com>
@bors
Copy link
Contributor

bors bot commented Dec 16, 2020

Build failed:

japaric
japaric reviewed Dec 16, 2020
View reviewed changes
macros/src/lib.rs Outdated Show resolved Hide resolved
@japaric
Copy link
Member

japaric commented Dec 16, 2020

Thanks! (and sorry for the repeated rebase work)

bors r+

@Sh3Rm4n
Copy link
Contributor Author

Sh3Rm4n commented Dec 16, 2020

No problem, these were minor anyways. :)

@bors
Copy link
Contributor

bors bot commented Dec 16, 2020

Build succeeded:

@bors bors bot merged commit 694bf31 into knurling-rs:main Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@japaric japaric japaric left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

derive(Format): support more than 256 variants
2 participants
@Sh3Rm4n @japaric