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

Add a fwupd plugin for ASUS ROG ally and ROG ally X #7782

Merged
merged 2 commits into from
Nov 13, 2024
Merged

Conversation

superm1
Copy link
Member

@superm1 superm1 commented Sep 24, 2024

This is only for querying the MCU firmware version for now. The sequence for writing the firmware update either needs some more RE or some help from ASUS.

@flukejones can I please ask you to test this branch on Ally and Ally X and show the versions it pulls up to see if everything looks sane?

@superm1 superm1 force-pushed the superm1/asus-hid branch 2 times, most recently from b91b243 to eaca806 Compare September 24, 2024 19:27
@superm1
Copy link
Member Author

superm1 commented Sep 24, 2024

I received an email confirming this is working. Here was the device ID that shows up:

├─N-KEY Device:
│ │ Device ID: b8a2645298053fb62ea03e27feea6c483d3fd27e
│ │ Current version: 0.2
│ │ Vendor: ASUSTek Computer, Inc. (USB:0x0B05)
│ │ GUID: a42577e5-92b2-53df-a7d1-513812a2ea4f ← USB\VID_0B05&PID_1B4C
│ │
│ └─Microcontroller 0:
│ Device ID: 669b88991d577a8d22a508e5dcbc798726edbf90
│ Current version: 312
│ Vendor: ASUSTek Computer, Inc. (USB:0x0B05)
│ GUID: 9a88fe8b-e444-595e-848e-c1937c54669e ← USB\VID_0B05&PID_1B4C&PART_RC72LA

@flukejones
Copy link

There is an issue where doing the probe now causes the Ally X MCU to start a vibration command and it won't stop until I remove/add my ally driver and then suspend/resume.

This vibration continues even in suspended state.

@superm1 I think the last version that worked fine was a8692e9 with the Ally X added to quirk

@superm1
Copy link
Member Author

superm1 commented Sep 24, 2024

There is an issue where doing the probe now causes the Ally X MCU to start a vibration command and it won't stop until I remove/add my ally driver and then suspend/resume.

This vibration continues even in suspended state.

@superm1 I think the last version that worked fine was a8692e9 with the Ally X added to quirk

Can you try taking out the command that is setting the manufacturer ID? I suspect that's what's doing it and that it happens to do more than we thought

@flukejones
Copy link

It seems I didn't notice this before as the vibration issue occurs on on AC plugged in. I'll try the suggestion now.

@flukejones
Copy link

@superm1 whcih command is this exactly? I'm not familiar with how fwupd works.

fu_device_set_proxy(dev_tmp, device);
if (!fu_asus_hid_device_ensure_version(dev_tmp, i, error))
return FALSE;
if (!fu_asus_hid_device_ensure_manufacturer(dev_tmp, error))
Copy link
Member Author

Choose a reason for hiding this comment

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

@flukejones comment this function call out and see if it helps.

@flukejones
Copy link

Output:

├─N-KEY Device:
│ │   Device ID:          b8a2645298053fb62ea03e27feea6c483d3fd27e
│ │   Current version:    0.2
│ │   Vendor:             ASUSTek Computer, Inc. (USB:0x0B05)
│ │   GUID:               a42577e5-92b2-53df-a7d1-513812a2ea4f ← USB\VID_0B05&PID_1B4C
│ │
│ └─Microcontroller 0:
│       Device ID:        669b88991d577a8d22a508e5dcbc798726edbf90
│       Current version:  312_T01
│       Vendor:           ASUSTek Computer, Inc. (USB:0x0B05)
│       GUID:             9a88fe8b-e444-595e-848e-c1937c54669e ← USB\VID_0B05&PID_1B4C&PART_RC72LA

Commenting the thing didn't help. But I've done now a full reset on the repo and rebuilt - it seems that another command is resetting the MCU as I'm seeing the LEDs reset to static-mode colour.

@superm1
Copy link
Member Author

superm1 commented Sep 24, 2024

Output:

├─N-KEY Device:
│ │   Device ID:          b8a2645298053fb62ea03e27feea6c483d3fd27e
│ │   Current version:    0.2
│ │   Vendor:             ASUSTek Computer, Inc. (USB:0x0B05)
│ │   GUID:               a42577e5-92b2-53df-a7d1-513812a2ea4f ← USB\VID_0B05&PID_1B4C
│ │
│ └─Microcontroller 0:
│       Device ID:        669b88991d577a8d22a508e5dcbc798726edbf90
│       Current version:  312_T01
│       Vendor:           ASUSTek Computer, Inc. (USB:0x0B05)
│       GUID:             9a88fe8b-e444-595e-848e-c1937c54669e ← USB\VID_0B05&PID_1B4C&PART_RC72LA

Commenting the thing didn't help. But I've done now a full reset on the repo and rebuilt - it seems that another command is resetting the MCU as I'm seeing the LEDs reset to static-mode colour.

I wonder if it's actually the opening and closing of the hid endpoint doing this.

@flukejones
Copy link

@superm1

I wonder if it's actually the opening and closing of the hid endpoint doing this.

The behaviour I see seems to be consistent for this. From memory it's when the HID endpoint is opened, I have to send a packet to the device to stop vibration when the hid-asus-ally driver first loads.

@superm1
Copy link
Member Author

superm1 commented Sep 25, 2024

@superm1

I wonder if it's actually the opening and closing of the hid endpoint doing this.

The behaviour I see seems to be consistent for this. From memory it's when the HID endpoint is opened, I have to send a packet to the device to stop vibration when the hid-asus-ally driver first loads.

Can you point me at the packet? I can try to send the same thing on fwupd open.

@superm1
Copy link
Member Author

superm1 commented Sep 25, 2024

@superm1

I wonder if it's actually the opening and closing of the hid endpoint doing this.

The behaviour I see seems to be consistent for this. From memory it's when the HID endpoint is opened, I have to send a packet to the device to stop vibration when the hid-asus-ally driver first loads.

Can you point me at the packet? I can try to send the same thing on fwupd open.

Is it this stuff?

static int ally_gamepad_init(struct hid_device *hdev, u8 report_id)
{
	const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, 0x65,
			   0x63,      0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
	int ret;

	ret = asus_dev_set_report(hdev, buf, sizeof(buf));
	if (ret < 0)
		hid_err(hdev, "Ally failed to send init command: %d\n", ret);

	return ret;
}

@flukejones
Copy link

	report->report_id = 0x0D;
	report->ff.enable = 0x0F; /* Enable all by default */
	report->ff.pulse_sustain_10ms = 0xFF; /* Duration */
	report->ff.pulse_release_10ms = 0x00; /* Start Delay */
	report->ff.loop_count = 0xEB; /* Loop Count */

I think a HID data packet of 0x0D, 0x0F, 0x00, 0x00, 0x00 should work.

@flukejones
Copy link

flukejones commented Sep 25, 2024

@superm1
Is it this stuff?

static int ally_gamepad_init(struct hid_device *hdev, u8 report_id)
{
	const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, 0x65,
			   0x63,      0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
	int ret;

	ret = asus_dev_set_report(hdev, buf, sizeof(buf));
	if (ret < 0)
		hid_err(hdev, "Ally failed to send init command: %d\n", ret);

	return ret;
}

No that never stopped the vibration. Have to send the one above. It's possible I'm missing something else in the init but I've never got around to seeing what it is.

The ASUS app sends an absolute ton of stuff.

But for this issue I think we need to look at the MCU update capture to identify what might come after.

@superm1
Copy link
Member Author

superm1 commented Sep 25, 2024

I think a HID data packet of 0x0D, 0x0F, 0x00, 0x00, 0x00 should work.

Hmm, I don't see any write to report 13 in the dump you shared me. Perhaps this means more of the sequence of HID commands before getting the FW version may be needed?

@flukejones
Copy link

Most likely. I am fairly sure there is one that perhaps stops the dev from doing things? Really need an answer from asus about now

@superm1
Copy link
Member Author

superm1 commented Sep 25, 2024

I spent some time looking at the dll and pulled out some other commands. Will need to see if any of these show up before the get fw version from the dumps.

@flukejones
Copy link

I have a brief response from ASUS that they are studying update process and will get back to us soon, possibly in the email chain between us and Richard.

Below are some logs that will help match things up. I think the first one matches the pcap. And I'm willing to bet the bin file bytes would match the pcap data. I'll try to find those bin files again.

ROGKBFWUpdateTool_20231105122303.log
ROGKBFWUpdateTool_20231105160506.log

@hughsie
Copy link
Member

hughsie commented Sep 25, 2024

Great work everybody!

@superm1
Copy link
Member Author

superm1 commented Sep 25, 2024

I have a brief response from ASUS that they are studying update process and will get back to us soon, possibly in the email chain between us and Richard.

Below are some logs that will help match things up. I think the first one matches the pcap. And I'm willing to bet the bin file bytes would match the pcap data. I'll try to find those bin files again.

ROGKBFWUpdateTool_20231105122303.log ROGKBFWUpdateTool_20231105160506.log

Can you add a log for Ally X? It seems for Ally that there is a set of flows that does second IC first, flash reset, then first IC and switch to main rom.

And if you can get a packet capture from Ally X (ideally through a VM if you can) it would be good to put it all together.

@flukejones
Copy link

flukejones commented Sep 26, 2024

ally-x-312_t01_update.log

Full capture data used. The log matches the pcap here, and the binary data also.

I did spot what looks like a "boot" command, lists the device in the string also as UUIT8297-BOOT-V1.1 in Packet 364 in response a 0xD1 packet. Oh yeah, this data looks like exactly what is in the bin file, so 0xD1 must be a "read me this block", and 0xC1 seems like a "write this block", and that data looks like a different format?

For that vibe issue: might be we need to send the ASUS Tech.Inc. data (initialise) after reading version. Or after the endpoint is opened before the version request.

NOTE: *the data read back in the pcap is the exact data in linked bin file since I forced an overwrite of same version. 0xD1 is hid page, second byte is the block address? I see FGA80100.RC72LA.312 is also in the bin file as data.

@flukejones
Copy link

I will get a capture of a downgrade to an earlier firmware so we can be double clear on what is what.

@superm1
Copy link
Member Author

superm1 commented Sep 26, 2024

Full capture data used. The log matches the pcap here, and the binary data also.

Man, unfortunately ./contrib/pcap2emulation.py doesn't want to convert the packet capture to fwupd format.
@fdanis-oss any ideas what is wrong with it?

Good news though, the new firmware parser I put in which pulls the string from the right offset in the binaries seems to be working both on Ally and Ally-X binaries!

❯ fwupdtool firmware-parse ~/Downloads/FGA80100.RC72LA.312.bin asus-hid
Loading…                 [************************************** ]
<firmware gtype="FuAsusHidFirmware">
  <data size="0x40000">[GInputStream]</data>
  <fga>FGA80100</fga>
  <product>RC72LA</product>
  <version>312</version>
</firmware>
❯ fwupdtool firmware-parse ~/src/asusmcu/ROGMCUFWUpdateTool/fw/FGA80100.RC71LS.318_T02.bin asus-hid
Loading…                 [************************************** ]
<firmware gtype="FuAsusHidFirmware">
  <data size="0x1e000">[GInputStream]</data>
  <fga>FGA80100</fga>
  <product>RC71LS</product>
  <version>318_T02</version>
</firmware>
❯ fwupdtool firmware-parse ~/src/asusmcu/ROGMCUFWUpdateTool/fw/FGA80100.RC71LM.318_T02.bin asus-hid 
Loading…                 [************************************** ]
<firmware gtype="FuAsusHidFirmware">
  <data size="0x1b000">[GInputStream]</data>
  <fga>FGA80100</fga>
  <product>RC71LM</product>
  <version>318_T02</version>
</firmware>

For that vibe issue: might be we need to send the ASUS Tech.Inc. data (initialise) after reading version. Or after the endpoint is opened before the version request.

I've been wondering about that, in the Ghidra stuff and packet capture it looked at first like random stale stack data, but it really does have a static string. If it's functional then yeah I should be able to force it in to see if it helps. I'll do that tomorrow.

@NeroReflex
Copy link

Capture shared. Cloud recovery did not work and I simply reinstall ed windiws the usual way.
Also yeah usbpcap refused to work in a win to go environment

@superm1
Copy link
Member Author

superm1 commented Oct 21, 2024

Thanks @NeroReflex! I've pulled it into my branch of emulation data and will continue to work with it.

@superm1
Copy link
Member Author

superm1 commented Oct 23, 2024

With my cleaned up emulation data at https://github.com/superm1/ally-emulation I've managed to do a full write sequence for the microcontroller RC71LS, but not for RC71LM.
I still need to dig into whether it's an emulation data capture problem or plugin problem with offsets for RC71LM.

@hughsie I was about to rebase, but with the changes in 88050ee I wanted to check how you think this should be handled. I need to send a few commands at the start of the page, write all the blocks, send a command at end of page. So walking a straightforward loop doesn't work unless I special case. Thoughts? Or maybe if you have an idea just (force) push to the branch.

@hughsie
Copy link
Member

hughsie commented Oct 23, 2024

So walking a straightforward loop doesn't work unless I special case.

I think you're going to need a FuChunkArray of a FuChunkArray still - you need to do (failable) stuff at the start and end of the page. I've got a branch which adds the fu_chunk_get_stream() API you need, but it depends on the steelseries rustgen branch. If you want a sneak peak it's predictably hughsie/fu_chunk_stream_new

@superm1
Copy link
Member Author

superm1 commented Oct 23, 2024

So walking a straightforward loop doesn't work unless I special case.

I think you're going to need a FuChunkArray of a FuChunkArray still - you need to do (failable) stuff at the start and end of the page.

OK thanks, that clarifies my big confusion, I didn't see how this API could possibly work for that purpose without a FuChunkArray of a FuChunkArray.

I've got a branch which adds the fu_chunk_get_stream() API you need, but it depends on the steelseries rustgen branch. If you want a sneak peak it's predictably hughsie/fu_chunk_stream_new

Ack, as I still haven't verified RC71LM, I'll hold off on rebasing. Maybe by the time I'm ready that will have landed :)

@superm1 superm1 force-pushed the superm1/asus-hid branch 4 times, most recently from d8ff5e7 to abc347a Compare November 9, 2024 20:54
@hughsie
Copy link
Member

hughsie commented Nov 12, 2024

@superm1 I'd be fine if you wanted to merge this to avoid it bitrotting. We can always disable it by default in the build if you want.

@superm1
Copy link
Member Author

superm1 commented Nov 12, 2024

@superm1 I'd be fine if you wanted to merge this to avoid it bitrotting. We can always disable it by default in the build if you want.

That's not a bad idea to merge as is with some guard rail. I don't mind it being enabled by default, enumeration is definitely correct.

I am thinking to add a force check for the write routine though. Once I find the time to find the problem with the other microcontroller can drop that. I'll spin a change.

@superm1 superm1 marked this pull request as ready for review November 12, 2024 14:50
@superm1 superm1 requested a review from hughsie November 12, 2024 14:50
static void
fu_asus_hid_device_init(FuAsusHidDevice *self)
{
fu_device_add_private_flag(FU_DEVICE(self), FU_DEVICE_PRIVATE_FLAG_NO_AUTO_REMOVE_CHILDREN);
Copy link
Member

Choose a reason for hiding this comment

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

do you need the children to outlive the parent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup! Specifically when doing the detach/attach dance the parent disappears. Without those children you won't have all the details of what payload goes to what child device.

hughsie
hughsie previously approved these changes Nov 12, 2024
@superm1
Copy link
Member Author

superm1 commented Nov 12, 2024

Thx, let's hold off to merge 1-2 days more. I need to make sure I didn't break the installation emulation that was working so far with any of these changes for child vs parent ownership.

Until this has been validated end to end either on real hardware or
on emulation the plugin should not be allowed to issue upgrades.

Leave the flag in place however for testing with --force.
@superm1
Copy link
Member Author

superm1 commented Nov 13, 2024

Yup, found some regression. I've sorted it out and can get the secondary controller updating again.

│ ├─Microcontroller 0:
│ │     Device ID:        105a998eae01e451237dc37974beab9362f0066f
│ │     Current version:  319
│ │     Vendor:           ASUSTek Computer, Inc. (USB:0x0B05)
│ │     Update State:     Success
│ │     Last modified:    2024-11-13
│ │     GUIDs:            f7765461-3ff4-52a0-a4d8-d782b1f885ab ← USB\VID_0B05&PID_1ABE&PART_RC71LS
│ │                       5f0738ae-5e3a-5e94-99fb-62d1b0fefcb1
│ │                       571229d5-caed-5cbc-91c1-5c216601a194
│ │                       8eeac4f3-d98d-57aa-a1fc-b44e8d3d8ba3
│ │                       c33b161d-a538-57df-b1c8-90817ada2387
│ │     Device Flags:     • Updatable
│ │                       • Unsigned Payload
│ │                       • Emulated

The primary controller update flow is missing from the emulation data but I'm suspecting a conversion issue because it's quite obviously in the wireshark dump.

@superm1 superm1 merged commit 6d77936 into main Nov 13, 2024
19 checks passed
@superm1 superm1 deleted the superm1/asus-hid branch November 13, 2024 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants