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

Refactor rtt [1/2] #574

Merged
merged 4 commits into from
Sep 17, 2021
Merged

Refactor rtt [1/2] #574

merged 4 commits into from
Sep 17, 2021

Conversation

Urhengulas
Copy link
Member

@Urhengulas Urhengulas commented Sep 8, 2021

First half of #573:

  • defmt-rtt: Move struct Channel into own module
  • defmt-rtt: Make fn host_is_connected a method of Channel
  • defmt-rtt: Move struct Header into own module
  • defmt-rtt: Rename fn handle to fn channel + mv to mod channel

@jonas-schievink
Copy link
Contributor

Looks good! Although I think I'd prefer to drop 6874e9b, since that moves a lot more than just the Header struct, and the _SEGGER_RTT static is something that IMO doesn't belong buried in a header module. (this also needs a rebase after the recent RTT fixes, but other than that LGTM)

@Urhengulas
Copy link
Member Author

Looks good! Although I think I'd prefer to drop 6874e9b, since that moves a lot more than just the Header struct, and the _SEGGER_RTT static is something that IMO doesn't belong buried in a header module. (this also needs a rebase after the recent RTT fixes, but other than that LGTM)

So - I would keep the consts and statics in the lib.rs and only move out the struct Header.

@Urhengulas
Copy link
Member Author

@jonas-schievink Can you please take another look?

@jonas-schievink
Copy link
Contributor

What do we gain from moving Header to another file? Since it only has 4 field and no methods, it doesn't use much code

@Urhengulas
Copy link
Member Author

What do we gain from moving Header to another file? Since it only has 4 field and no methods, it doesn't use much code

Actually true

@Urhengulas
Copy link
Member Author

That's it?

@jonas-schievink
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 17, 2021

Build succeeded:

@bors bors bot merged commit 96a3043 into main Sep 17, 2021
@bors bors bot deleted the refactor-rtt-1 branch September 17, 2021 17:23
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