-
Notifications
You must be signed in to change notification settings - Fork 79
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
Rust tools for working with Realtek firmware #261
Conversation
Here's the current subcommand hierarchy. Is this intuitive? Does it leave the necessary conceptual room for more tools?
|
@@ -61,9 +61,8 @@ async def do_load(usb_transport, force): | |||
# Get the driver. | |||
driver = await rtk.Driver.for_host(host, force) | |||
if driver is None: | |||
if not force: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's erroneous to only return if not force
-- otherwise, it would then try to driver.download_firmware()
, which would surely fail if driver is None
.
rust/src/wrapper/drivers/rtk.rs
Outdated
} | ||
if opcode == 0 && length == 1 { | ||
project_id = offset | ||
.checked_sub(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The equivalent logic in python doesn't do this check before reading offset - 1
, so I think on a suitably mangled file it could read from the header instead of stopping short.
c891a57
to
a403f79
Compare
rust/src/wrapper/drivers/rtk.rs
Outdated
// look for project id from the end | ||
let mut offset = payload.len(); | ||
let mut project_id: Option<u8> = None; | ||
while offset >= 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that a project ID can be found within a patch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming that there is some undocumented footer of opcodes after the patches, but I don't know for sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the opcodes are to be found after the patches (or not at all), then it might be more correct to only check the post-patch bytes (if present).
But I recognize that this is copying what the Python implementation does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, we could chop the search down to only the space after patches, but unless we know that's actually valid, I don't think it makes sense to do anything other than blindly follow in the footsteps of the Python impl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any insights @barbibulle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code got moved; it's now at https://github.com/google/bumble/pull/261/files#diff-1a70751eee41dc84f6ba4d3247b476d244e914ad403c219c88d4c154f9b0c991R103.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The python implementation just mirrors the Linux driver implementation, where looking for the project ID is here: https://github.com/torvalds/linux/blob/99d99825fc075fd24b60cc9cf0fb1e20b9c16b0f/drivers/bluetooth/btrtl.c#L677
And you're right that the search for opcodes should stop at the end of the patch block. I think the C implementation takes the shortcut of assuming that there will always be an opcode list with at least an end
opcode, so it cheats a bit by not calculating the size of the patch block. There is still a check to stop at the end of the header, because that's easy, fixed size, and that ensures the search doesn't get out of bounds of the firmware buffer at least. But a malformed file with no end
or project_id
opcode after the patch block could lead to incorrectly finding a lookalike inside the patch block.
a403f79
to
6e6b762
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approval with the assumption that
rust/src/wrapper/drivers/rtk.rs
Outdated
// look for project id from the end | ||
let mut offset = payload.len(); | ||
let mut project_id: Option<u8> = None; | ||
while offset >= 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any insights @barbibulle?
611276d
to
72712d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approval with the assumption that the rtk firmware parsing in Python is also correct (aside from the unchecked sub when getting project_id
)
@@ -66,3 +69,21 @@ async def get_driver_for_host(host): | |||
return driver | |||
|
|||
return None | |||
|
|||
|
|||
def project_data_dir() -> pathlib.Path: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSOn KeyStore implementation currently uses appdirs
for this prupose, but I see that the community is moving to platformdirs
instead, as you are doing here, so I'll make the change in JsonKeyStore to switch to that as well.
72712d0
to
7e8d43f
Compare
Further adventures in porting tools to Rust to flesh out the supported API. These tools didn't feel like `example`s, so I made a top level `bumble` CLI tool that hosts them all as subcommands. I also moved the usb probe not-really-an-`example` into it as well. I'm open to suggestions on how best to organize the subcommands to make them intuitive to explore with `--help`, and how to leave room for other future tools. I also adopted the per-OS project data dir for a default firmware location so that users can download once and then use those .bin files from anywhere without having to sprinkle .bin files in project directories or reaching inside the python package dir hierarchy.
7e8d43f
to
0e2fc80
Compare
Further adventures in porting tools to Rust to flesh out the supported API.
These tools didn't feel like
example
s, so I made a top levelbumble
CLI tool that hosts them all as subcommands. I also moved the usb probe not-really-an-example
into it as well. I'm open to suggestions on how best to organize the subcommands to make them intuitive to explore with--help
, and how to leave room for other future tools.I also adopted the per-OS project data dir for a default firmware location so that users can download once and then use those .bin files from anywhere without having to sprinkle .bin files in project directories or reaching inside the python package dir hierarchy.