Skip to content

Conversation

@tullom
Copy link
Contributor

@tullom tullom commented Jul 7, 2025

Thank you to @magravel for fixing the changes in embedded-cfu-protocol. This PR includes the same .rs changes in #341.

@tullom tullom force-pushed the cfu-v-0-2-0 branch 3 times, most recently from 6f00f26 to 56f07c6 Compare July 7, 2025 23:41
@tullom tullom changed the title Test CFU fix Fix CFU breaking changes, update battery-service device to use Mutex, and update examples Jul 8, 2025
@tullom tullom added BREAKING CHANGE Marks breaking changes enhancement New feature or request cfu labels Jul 8, 2025
@tullom tullom self-assigned this Jul 8, 2025
@tullom tullom moved this to In review in Embedded Controller Jul 8, 2025
@tullom tullom marked this pull request as ready for review July 8, 2025 00:17
@tullom tullom requested a review from a team as a code owner July 8, 2025 00:17
@kurtjd
Copy link
Contributor

kurtjd commented Jul 8, 2025

The changes themselves look good to me but the commit structure is a bit odd IMO. I'd suggest the first commit be renamed to something more descriptive than "Test", and it also fails to build (all commits should be able to build independently).

The second commit could maybe be split into several others as it seems to handle a few different things but that's more of a nit.

@tullom
Copy link
Contributor Author

tullom commented Jul 8, 2025

The changes themselves look good to me but the commit structure is a bit odd IMO. I'd suggest the first commit be renamed to something more descriptive than "Test", and it also fails to build (all commits should be able to build independently).

The second commit could maybe be split into several others as it seems to handle a few different things but that's more of a nit.

@kurtjd Good catch, this PR was originally intended to be a test PR but it evolved into a full blown change. Renamed the commits to be more descriptive. The first commit is all cargo updates and CFU changes, second commit is battery changes. The commits will be squashed anyways so this separation of commits is just for ease of review.

@kurtjd
Copy link
Contributor

kurtjd commented Jul 8, 2025

The changes themselves look good to me but the commit structure is a bit odd IMO. I'd suggest the first commit be renamed to something more descriptive than "Test", and it also fails to build (all commits should be able to build independently).
The second commit could maybe be split into several others as it seems to handle a few different things but that's more of a nit.

@kurtjd Good catch, this PR was originally intended to be a test PR but it evolved into a full blown change. Renamed the commits to be more descriptive. The first commit is all cargo updates and CFU changes, second commit is battery changes. The commits will be squashed anyways so this separation of commits is just for ease of review.

Ah yeah, forgot that we squash commits, I apologize. But looks good!

@kurtjd kurtjd merged commit d646d2f into OpenDevicePartnership:main Jul 8, 2025
21 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Embedded Controller Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BREAKING CHANGE Marks breaking changes cfu enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants