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

Global GPIO/Pin Migration #584

Merged

Conversation

stellar-aria
Copy link
Contributor

This continues the work started in #581, updating references to dsy_gpio_pin and dsy_gpio to instead use the new C++ style GPIO and Pin classes.

As per the discussion with @stephenhensley, this is a breaking change for board header files (their GPIO variables are now updated), but does not include any of the commits removing the now-obsolete, deprecated code. As such, the API as a whole remains largely unaltered. The commits dealing with code removal will be submitted in a future PR once the documentation work discussed has been completed and preparations begin for a new major revision.

Minor breaking API changes:

DaisySeed, DaisyPatch, daisy::patch_sm, and DaisyField now use GPIO instead of dsy_gpio.

This also finally switches the AK4556 CODEC from a static set of functions to
instance variables. This is required due to how GPIO instances update their
internal port_base_addr_ on Init(), which is then later required for DeInit().
Also migrates from internal custom Pull representation to the global GPIO::Pull
@stephenhensley
Copy link
Collaborator

Hi @stellar-aria

Thanks for another stellar (pardon the pun) PR.
Quickly looking through everything I think everything looks good.
So exciting to see some of the oldest bits of the library catching up with the times.

There are a few more in-progress PRs that will get wrapped up/merged within the next week (MIDI fixes, the ADC speed feature, etc.) as part of a new minor release.
Once that's out, I think this would be a good first step toward the larger-scope cleanup including the HAL update in #582, etc. that we'd been discussing.

I'll keep you updated if we find anything here that needs changing, but unless anything got missed, or there are any typos, I think this might just be good to go.

@stellar-aria
Copy link
Contributor Author

Just merging in the upstream changes to make sure everything integrates cleanly. Had to do some conflict resolution to get everything to mesh smoothly.

@stellar-aria stellar-aria force-pushed the feature/global-gpio-migration branch from 3f51516 to 0100588 Compare June 19, 2023 20:08
@stellar-aria stellar-aria force-pushed the feature/global-gpio-migration branch from 0100588 to 6567ad7 Compare June 19, 2023 20:09
@stellar-aria
Copy link
Contributor Author

This now matches up with the latest master and should be a clean merge.

@stephenhensley
Copy link
Collaborator

@stellar-aria thanks for syncing this up! We're starting this week on review/testing on this one (should go a lot faster than the HAL stuff).

We'll keep you posted if anything comes up.

Also:
Added a deprecation warning to the dsy_gpio_pin struct
Left out some of the additional stuff surrounding that struct,
e.g. comparison operators, implicit cast from Pin to dsy_gpio_pin
These would throw the deprecation warning, so theyve been left out
I also added an implicit cast from dsy_gpio_pin to daisy::Pin
This required moving the dsy_gpio_pin and port inside the daisy namespace
@beserge
Copy link
Contributor

beserge commented Feb 27, 2024

Made some changes:

  • Used the new Pin class exclusively in the DaisySeed class
  • Change the GetPin functions in DaisySeed and DaisyPatchSM to return daisy::Pin
  • Remove the old version of the hal_map utils
  • Remove the old dsy_gpio_pin based daisy::GateIn::Init()
  • Remove the dsy_gpio_pin based functions from gpio.cpp. This version will only support using dsy_gpio_pin as a temporary struct to then be immediately converted to a daisy::Pin
  • Make some changes to dsy_gpio_pin as pre deprecation steps
    • Mark dsy_gpio_pin as deprecated, this throws warnings in gcc now if you use it
    • Get rid of some of the helpers for dsy_gpio_pin and the implicit conversion from Pin->dsy_gpio_pin. These were triggering the deprecation warnings, and shouldn't be needed at this point anyways.
    • Add an implicit conversion from dsy_gpio_pin -> daisy::Pin. This required moving dsy_gpio_pin and port into the daisy namespace.
  • Fixed issues with the Unit Tests
  • Created Fix non-building examples DaisyExamples#299

Remove test making sure new pin could cast to old
Add two conditionals so the hal_map wont be included during some tests
@beserge
Copy link
Contributor

beserge commented Feb 27, 2024

Other than the stuff I edited, I went through and tested that everything works as intended. I'm ready to sign off! :shipit:

@stephenhensley
Copy link
Collaborator

So as a final test I upgraded a bigger application (bootloader, USB, LED drivers, SD Card, Shift Registers, ADCs, etc.) and the only issue I ran into was that the timing for the CD4021 was slightly different using all of the new functions, and the "1-tick" the timing was already on the edge of what the chip could accept when powered at 3v3.

With that said, that issue isn't specific to this PR. So I'm going to just add increase that delay (and probably make it an optional config value) in a separate PR.

Sorry that this PR took so long to merge!

@stephenhensley stephenhensley merged commit f7dd38f into electro-smith:master Sep 20, 2024
4 checks passed
@stellar-aria stellar-aria deleted the feature/global-gpio-migration branch October 19, 2024 16:51
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.

3 participants