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

Bootloader: Add Reading of Git SHA to bootloader #28492

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joshanne
Copy link
Contributor

Allows the bootloader build to be identified with uploader.py

@joshanne
Copy link
Contributor Author

This is helpful for users building their own bootloaders, and allowing the user to identify what commit the build was built at.

Feedback welcome.

@joshanne joshanne force-pushed the pr/git-hash-in-bootloader branch from da23530 to 29788c9 Compare October 29, 2024 19:32
@@ -128,6 +128,7 @@
#define PROTO_DEVICE_FW_SIZE 4 // size of flashable area
#define PROTO_DEVICE_VEC_AREA 5 // contents of reserved vectors 7-10
#define PROTO_DEVICE_EXTF_SIZE 6 // size of available external flash
#define PROTO_DEVICE_BL_GIT_HASH 7 // git hash of bootloader build
Copy link
Contributor Author

@joshanne joshanne Oct 29, 2024

Choose a reason for hiding this comment

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

I've also got changes that add reading off the Application Git SHA if AP_CHECK_FIRMWARE_ENABLED is enabled in the bootloader (and application).

We certainly would find these handy, just a question of if this is desirable in the bootloader.

@joshanne joshanne requested a review from peterbarker October 30, 2024 02:17
Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

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

great idea, that would prevent to always have to update the bootloader when you do custom one's !

@joshanne joshanne force-pushed the pr/git-hash-in-bootloader branch from 29788c9 to ddfcf85 Compare October 31, 2024 01:56
@joshanne joshanne force-pushed the pr/git-hash-in-bootloader branch from ddfcf85 to 8b6bb5c Compare October 31, 2024 05:56
Tools/scripts/uploader.py Outdated Show resolved Hide resolved
@tridge
Copy link
Contributor

tridge commented Nov 5, 2024

if we can get away without a protocol change, by checking for a NACK then that would be less risky, otherwise we need to be sure existing bootloader clients are OK

@peterbarker
Copy link
Contributor

Wrote this about 4 years ago - glad someone's pushing through with it (https://github.com/ArduPilot/ardupilot/pull/14768/files)

@joshanne joshanne force-pushed the pr/git-hash-in-bootloader branch 3 times, most recently from f78e3c6 to a2b09d6 Compare November 6, 2024 19:47
@joshanne
Copy link
Contributor Author

joshanne commented Nov 6, 2024

If this implementation stands, probably need to include an update to the comments above, here:

// get device info
//
// command: GET_DEVICE/<arg:1>/EOC
// BL_REV reply: <revision:4>/INSYNC/EOC
// BOARD_ID reply: <board type:4>/INSYNC/EOC
// BOARD_REV reply: <board rev:4>/INSYNC/EOC
// FW_SIZE reply: <firmware size:4>/INSYNC/EOC
// VEC_AREA reply <vectors 7-10:16>/INSYNC/EOC
// bad arg reply: INSYNC/INVALID

@joshanne
Copy link
Contributor Author

joshanne commented Nov 6, 2024

Wrote this about 4 years ago - glad someone's pushing through with it (https://github.com/ArduPilot/ardupilot/pull/14768/files)

@peterbarker - Your implementation was adding a new command. This implementation makes use of the GET_DEVICE (_INFO) command.

Do you have a feeling either way? I'm less inclined to burn a full command index unnecessarily.

@joshanne
Copy link
Contributor Author

joshanne commented Nov 6, 2024

@peterbarker @tridge - given this is based off PX4-Bootloader, what requirement is there to keep it in sync? would that be a sufficient reason to burn a command index, rather than reading additional device info?


default:
goto cmd_bad;
}
done_get_device_flags |= (1<<(arg-1)); // set the flags for use when resetting timeout
done_get_device_flags |= DEVICE_INFO_MASK(arg); // set the flags for use when resetting timeout
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should probably bump the size of device flags to uint16 or uint32 given done_get_device_flags is a bitmask, and this PR adds device info fields 7, 8 and 9.

@joshanne joshanne force-pushed the pr/git-hash-in-bootloader branch from a2b09d6 to 5f3be59 Compare November 6, 2024 21:47
@tridge
Copy link
Contributor

tridge commented Nov 8, 2024

given this is based off PX4-Bootloader, what requirement is there to keep it in sync?

doesn't need to be completely in sync, but we do need to ensure that flashing ArduPilot when px4 bootloader is used works and vice-versa
some flight controllers that usually run ArduPilot come with the px4 bootloader

@peterbarker
Copy link
Contributor

We should probably PR into the PX4 bootloader the additional "info" IDs so as not to cause collisions down the road. Would require a chunk of documentation of the shape of the relevant data...

@joshanne joshanne force-pushed the pr/git-hash-in-bootloader branch from 5f3be59 to 1863c6a Compare January 7, 2025 23:28
@joshanne
Copy link
Contributor Author

joshanne commented Jan 8, 2025

We should probably PR into the PX4 bootloader the additional "info" IDs so as not to cause collisions down the road. Would require a chunk of documentation of the shape of the relevant data...

Documented size and shape of data for Build SHAs - both BL and App SHAs are 64 bits each (16 hex characters each).

Also pushed a PR to PX4 to reserve and maintain compatability with PX4 bootloader.

Only catch at this stage is app_descriptor is a uint32_t sha - so for the time being, we reply with the SHA, with succeeding zeroes, and handle display in uploader.py.

@joshanne
Copy link
Contributor Author

joshanne commented Jan 8, 2025

given this is based off PX4-Bootloader, what requirement is there to keep it in sync?

doesn't need to be completely in sync, but we do need to ensure that flashing ArduPilot when px4 bootloader is used works and vice-versa some flight controllers that usually run ArduPilot come with the px4 bootloader

Tested flashing AP to PX4 and back to AP works as expected. The display of the SHAs and bootloader options only occurs when using --identify.

@peterbarker
Copy link
Contributor

Apparently PX4 might have added an equivalent feature into their bootloader. Might be worth checking how they worked it into the protocol and whether we can be compatible.

@joshanne joshanne force-pushed the pr/git-hash-in-bootloader branch from 85e9698 to 1d1e8e6 Compare January 8, 2025 20:37
@joshanne
Copy link
Contributor Author

joshanne commented Jan 8, 2025

Apparently PX4 might have added an equivalent feature into their bootloader.

Yeah, I had noticed that late yesterday too. grumble grumble.

I'm reducing the scope of this PR to solely implement the bootloader version reporting using the same command as PX4.

Works as is, ArduPilot reports the 16 char sha (eg: 1d1e8e6cbb906865), PX4 reports a custom string (eg: PX4BLv1.16.0g1abc27dd1f).

Should we prefix the sha to look something like the following: APBL.1d1e8e6cbb906865?
If so, is that preferred to be setup at build time (in boards.py)? or run time (in bootloader.cpp)?

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.

7 participants