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

Sidecar Mainboard FPGA: add proper c/d image #1906

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

Aaron-Hartwig
Copy link
Contributor

This fixes a problem I accidentally introduced back in 4b74549, meaning it was part of Sidecar release v1.0.29 and R11. There's no meaningful impact to customers from this problem.

In that update, I accidentally copied the Rev B bitstream to both sidecar_mainboard_rev_b.bit and sidecar_mainboard_rev_c_d.bit. The result is that rev C/D Sidecars are running the rev B FPGA image. In practice this means very little, as the only meaningful change in FPGA-land is that we added a 36th Ignition link, the link between the controller and the target on its own board. At this time nothing is paying attention to any of that except our loopback test fixture in manufacturing (where this problem was caught).

Obviously this highlights our current manual process of getting FPGA images into our Hubris image releases. We should certainly have a discussion about how to add automation (or at very least checks!) to make this process less error-prone!

@labbott @jclulow with my apologies, we will need to get this released and deployed into manufacturing. We can coordinate that in chat. In the meantime, I will instruct the test engineers to ignore Ignition Port 35 errors for now as that is caused by this bug.

@Aaron-Hartwig
Copy link
Contributor Author

before

git checkout 4b74549a66d58a1d11457c74e6966009dc18ecab
Previous HEAD position was 24194171 Bump hubtools version to 0.4.6 (#1900)
HEAD is now at 4b74549a Sidecar: Update FPGA to Quartz 8a2c75a (#1901)

md5sum drv/sidecar-mainboard-controller/sidecar_mainboard_controller_rev_b.bit 
29c4c522b47afaea016e9123596fb428  drv/sidecar-mainboard-controller/sidecar_mainboard_controller_rev_b.bit

md5sum drv/sidecar-mainboard-controller/sidecar_mainboard_controller_rev_c_d.bit 
29c4c522b47afaea016e9123596fb428  drv/sidecar-mainboard-controller/sidecar_mainboard_controller_rev_c_d.bit

after

md5sum drv/sidecar-mainboard-controller/sidecar_mainboard_controller_rev_b.bit 
29c4c522b47afaea016e9123596fb428  drv/sidecar-mainboard-controller/sidecar_mainboard_controller_rev_b.bit

md5sum drv/sidecar-mainboard-controller/sidecar_mainboard_controller_rev_c_d.bit 
247dc0e5bbcbb258fee601fa16aa4c3c  drv/sidecar-mainboard-controller/sidecar_mainboard_controller_rev_c_d.bit

@Aaron-Hartwig
Copy link
Contributor Author

Aaron-Hartwig commented Oct 18, 2024

I tested this fix on a Sidecar D in manufacturing. Here is the output from the offending commit:

aaron@sam ~ $ pfexec humility -p 0483:3754:0039001D4741500920383733 -a build-sidecar-d-image-default-4b74549.zip hiffy -c Ignition.all_port_state
humility: attached to 0483:3754:0039001D4741500920383733 via ST-Link V3
Ignition.all_port_state() => Ignition_all_port_state_REPLY { value: [ PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0),
PortState(0x103000005120301), <-- Port 34 showing a connection
PortState(0x0),               <-- port 35 showing nothing
PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0) ] }

Here is the output running an image from the commit prior to the offending commit:

aaron@sam ~ $ pfexec humility -p 0483:3754:0039001D4741500920383733 -a build-sidecar-d-image-default-2419417.zip hiffy -c Ignition.all_port_state
humility: attached to 0483:3754:0039001D4741500920383733 via ST-Link V3
Ignition.all_port_state() => Ignition_all_port_state_REPLY { value: [ PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0),
PortState(0x303000007120301), <-- Port 34 showing a connection
PortState(0x303000007120301), <-- Port 35 showing a connection
PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0) ] }

Then again, with this fix:

aaron@sam ~ $ pfexec ./humility -p 0483:3754:0039001D4741500920383733 -a build-sidecar-d-image-default-364af32.zip hiffy -c Ignition.all_port_state
humility: attached to 0483:3754:0039001D4741500920383733 via ST-Link V3
Ignition.all_port_state() => Ignition_all_port_state_REPLY { value: [ PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0),
PortState(0x303000007120301), <-- Port 34 showing a connection
PortState(0x303000007120301), <-- Port 35 showing a connection
PortState(0x0), PortState(0x0), PortState(0x0), PortState(0x0) ] }

@jclulow note that I'm running the final test with a locally built humility as Hubris main now requires support for v9.

Copy link
Contributor

@nathanaelhuffman nathanaelhuffman left a comment

Choose a reason for hiding this comment

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

lgtm

@Aaron-Hartwig Aaron-Hartwig merged commit a58bc46 into master Oct 18, 2024
106 checks passed
@Aaron-Hartwig Aaron-Hartwig deleted the aaron/correct-sidecar-image branch October 18, 2024 18:21
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