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 support for one-way FPGA advertisement pin #22

Merged
merged 8 commits into from
Mar 7, 2024

Conversation

mndza
Copy link
Contributor

@mndza mndza commented Aug 29, 2023

Apollo firmware will keep the USB switch handed over to the FPGA as long as the gateware keeps advertising its usage through the FPGA_ADV pin (FPGA_INT). When these messages stop arriving, Apollo will take over the port. If the ads resume, the port will not be handed off to the FPGA again until a "honor FPGA_ADV" (0xc2) vendor request arrives.

Additionally, the host tools can request a gateware to hand off the USB port to Apollo if the necessary request is available.

  • firmware: Adds HONOR_FPGA_ADV vendor request (0xc2)
  • apollo_fpga: Use REQUEST_APOLLO_ADV_STOP (0xf0) to enumerate Apollo

Merge in block with:

@mossmann
Copy link
Member

mossmann commented Oct 6, 2023

This started failing CI due to a tinyusb change in master. See 3802956 for how to fix it.

@mossmann
Copy link
Member

mossmann commented Oct 6, 2023

Should we also add a vendor request that asks firmware to hand off the USB port to the FPGA? If the advertising fails for any reason (e.g. older or broken gateware), it might be nice to be able to explicitly switch the port.

@martinling
Copy link
Member

Should we also add a vendor request that asks firmware to hand off the USB port to the FPGA? If the advertising fails for any reason (e.g. older or broken gateware), it might be nice to be able to explicitly switch the port.

Another reason this would be useful is that it would switch immediately, rather than potentially waiting some time for the next periodic advertising message.

@mndza
Copy link
Contributor Author

mndza commented Oct 9, 2023

Should we also add a vendor request that asks firmware to hand off the USB port to the FPGA? If the advertising fails for any reason (e.g. older or broken gateware), it might be nice to be able to explicitly switch the port.

Should this request hand off the USB port to the FPGA until the PROGRAM button is pressed?

Another reason this would be useful is that it would switch immediately, rather than potentially waiting some time for the next periodic advertising message.

The current vendor request will switch immediately in most cases. It will only have to wait if the vendor request is called before advertisement starts, which we are not doing.

Copy link
Member

@martinling martinling left a comment

Choose a reason for hiding this comment

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

I've been using this code and the accompanying greatscottgadgets/luna#215 in cynthion-test for some time now, and I'm satisfied with the functionality, but I'm concerned about how we expose the handoff request on the FPGA side.

Currently, this is implemented as a request with the recipient as the device, which means this feature occupies the 0xF0 request number for any top-level device to which the ApolloAdvertiser and its request handler are attached.

This may conflict with people trying to emulate existing devices, or developing their own devices, whilst using a shared port - particularly given that we also have this plan for automatically adding this feature to devices using the CONTROL port on Cynthion.

We'd previously discussed using a separate interface at the protocol level to separate out this functionality (see 2023-08-08 meeting notes), and I still think that's the right direction.

Doing this would require some additional work:

  • In the gateware, rather than just attaching the request handler, it would be necessary to allocate an interface number, generate an interface descriptor, append this descriptor to the configuration descriptor, and then attach the request handler with knowledge of its interface number, so that the handler can check if it is the recipient of a request.

  • The apollo host code would need to retrieve the active configuration descriptor provided by the gateware, find the interface descriptor corresponding to the Apollo stub interface, claim that interface from the OS, and then issue the request with that interface as the recipient.

However, we may want to merge this as-is, since it's currently our only solution for sharing the CONTROL port on Cynthion, and proceed with the above changes later.

@mndza
Copy link
Contributor Author

mndza commented Oct 9, 2023

I agree with your concerns. The configuration flash bridge uses the scheme that you mention, where the vendor request targets a specific interface. However, the configuration descriptor there is fixed.

Let me try to find a way to append this interface descriptor to an existing configuration descriptor.

@martinling
Copy link
Member

I also wonder whether the ApolloAdvertiser gateware should live here in the Apollo repo rather than in LUNA.

firmware/src/main.c Outdated Show resolved Hide resolved
Apollo firmware will keep the USB switch handed over to the FPGA as
long as the gateware keeps advertising its usage through the FPGA_ADV
pin. When these messages stop arriving, Apollo will take over the port.
If the advertisement resumes, the port will not be handed off to the
FPGA again until an "honor FPGA_ADV" (0xc2) vendor request arrives.
When this is added as a submodule to a design, an advertisement message
is sent periodically to Apollo. Apollo will take over the port when
these announcements are interrupted or when the PROGRAM button is
pressed.

An optional request handler is added: REQUEST_APOLLO_ADV_STOP (0xf0).
It returns the CONTROL port to Apollo by stopping announcements.
The host tools can request gateware to hand off the USB port to Apollo
if the necessary request is available (REQUEST_APOLLO_ADV_STOP).
firmware/src/vendor.c Outdated Show resolved Hide resolved
@martinling martinling mentioned this pull request Feb 15, 2024
Simplify the control flow and add error messages.
@mndza mndza force-pushed the fpga_adv branch 2 times, most recently from 443873b to 17b2eb8 Compare March 1, 2024 14:08
Requests handoff to Apollo using an existing device handle.
Copy link
Member

@martinling martinling left a comment

Choose a reason for hiding this comment

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

This is now working as expected on the update-dependencies branch of the Cynthion factory test.

I've made one small style suggestion but it's not critical.

Co-authored-by: Martin Ling <martin-github@earth.li>
@mossmann mossmann merged commit b994838 into greatscottgadgets:master Mar 7, 2024
3 checks passed
@antoinevg antoinevg mentioned this pull request Apr 16, 2024
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.

None yet

3 participants