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

Update Daisy board definitions to use new Pin system #581

Merged
merged 11 commits into from
Jun 13, 2023

Conversation

stellar-aria
Copy link
Contributor

Hi! I just got a Daisy Patch a few days ago, and while I was creating a variant of the Daisy Patch class I noticed the original didn't use any of the new Pin system.

This seemed like a nice easy way to begin contributing, so I simply replaced all the pin macros with constexpr definitions and replaced the references accordingly. I did a simple test with the Patch's Sequencer program and it worked, for what that's worth.

This could be folded into a larger PR for doing this for all the boards, and I'd be more than happy to do so, but I only have the hardware to test things for the Patch and Seed.

Additionally, the dsy_gpio gate_output in the class could be replaced with a newer GPIO object, but that would be a breaking API change and I don't know what the procedures are for that/when it would be appropriate.

@stephenhensley
Copy link
Collaborator

Hi @stellar-aria

Thanks for the contribution!

I think everything looks good here.
Just the minor style-CI stuff.

Re. the gate outputs I've been trying to decide how to manage the same change for the GPIO output on the PatchSM (via #561)
Since a few of the other boards will require that same change we can probably do it separately, and all at once.

@stellar-aria
Copy link
Contributor Author

Yeah, that makes sense.

I'd be more than happy to do the other boards at the same time in this PR for completions sake if that'd be okay? I just have no way of physically testing them.

@stellar-aria
Copy link
Contributor Author

stellar-aria commented May 22, 2023

Oh, is the style CI a transformer automatically applied to the PR, or do I need to make changes to the relevant commit to have it match the CI output?

Edit: I've just gone ahead and amended the commit after running clang-format on the document. Don't know why I forgot to do that in the first place.

@stellar-aria stellar-aria force-pushed the daisy_pin_migration branch from 2f7ee10 to 42ef585 Compare May 22, 2023 15:14
@stellar-aria stellar-aria force-pushed the daisy_pin_migration branch from 42ef585 to bb63023 Compare May 29, 2023 04:33
This updates the patch_sm class to use the new Pin objects, which
necessitated restructuring the pin definitions in the header file in a
way that now exposes them similarly to the daisy::seed namespace, and
removes the need for the now-deprecated GetPin method.
@stellar-aria stellar-aria force-pushed the daisy_pin_migration branch 2 times, most recently from b508d8a to 5c30e60 Compare May 29, 2023 06:02
@stellar-aria
Copy link
Contributor Author

I've taken the liberty to adapt the rest of the board objects to Pin definitions, most of which is self-explanatory in the code. daisy::patch_sm was the only one that required more than simple editing, and the commit message explains the details.

This is all part of a larger (completed) rework to globally use GPIO and Pin, currently at stellar-aria:feature/global_gpio_migration that depends on the commits here.

@stellar-aria stellar-aria changed the title Update DaisyPatch to use new Pin system Update Daisy board definitions to use new Pin system May 29, 2023
@stephenhensley
Copy link
Collaborator

Hi @stellar-aria

Thanks for going above and beyond! For all of the DaisySeed based boards, everything looks good.
We'll do a quick set of hardware-tests early next week to go over everything, and make sure all of the hardware still works with the new mappings.

It looks like there was an issue with the conversion of the PatchSM pins, based on the CI build failing.
That said, I had done essentially the same as what you were doing in #561, which is just waiting on our decision for whether or not to update the gate outputs or not.

If you'd like you can integrate the changes in #561 here, or revert the PatchSM in this PR, and leave it to be handled in that PR.
Either way would be totally fine.

Re. the gate outputs:
We'll discuss what we want to do wrt the breaking change since that applies to the Patch, Field, and PatchSM boards.
I think the right thing to do is update it since the newer GPIO class is much more idiomatic to how everything else in libDaisy works, however we are cautious about breaking changes.

Regardless, we should be be finished with tests on Monday, and be ready to merge it.

This fixes breaking API changes due to the move of Pin definitions outside of DaisyPatchSM, by putting them back, albeit as constexpr definitions in the header file. kPinMap is still marked as deprecated when GetPin is removed.
@stellar-aria
Copy link
Contributor Author

Sorry, that was a mistake on my part, I had a different version locally with the pins as static members similar to the previous version that I just pushed up.

Are there plans to move daisy::patch_sm::DaisyPatchSM to just daisy::PatchSM at some point? The repo seems to be in a weird spot between moving from older C-style organization to namespaces.

Also, just a heads up that it looks like the repo's style linter is somehow following AllowShortFunctionsOnASingleLine as 'All' despite it being marked as Inline (i.e. only done when inside a class definition), possibly because of the lack of quotes. Is this an error on the part of the .clang-format style definition or on the part of the ci formatter? Basically, should the formatter be turning any function definition that is short enough into a single line, regardless of scope and location?

I think moving to Pin/GPIO throughout the library probably constitutes a large enough API change that might be worth a major version update. A good chunk of the examples still compile, but others such as patch/Sequencer still use things like dsy_gpio_write. A major version update would also allow for excising all the old C-style functions that are no longer needed for backwards compatibility from the API, such as dsy_gpio_write.

Heck or the GPIO change could be a minor version update, and the cleanup could be part of the major release?

Anyways, I'll open a PR for those changes once this one closes.

@stephenhensley
Copy link
Collaborator

@stellar-aria

Are there plans to move daisy::patch_sm::DaisyPatchSM to just daisy::PatchSM at some point? The repo seems to be in a weird spot between moving from older C-style organization to namespaces.

I actually like this idea a lot. Definitely would have to be a part of a major version update since it will require all previous code to change, albeit a simple change.

Brief history for context: libDaisy started out in C, and prior to the release of the Daisy shifted to C++, but a lot of the "guts", and earliest components of the library have stuck to the older conventions.

Also, just a heads up that it looks like the repo's style linter is somehow following AllowShortFunctionsOnASingleLine as 'All' despite it being marked as Inline

Hmm, I don't think that is intentional, but missing quotes might explain the occasional mismatch between local clang-format runs, and the CI script.
Personally, I actually think I prefer "All" in most cases, esp. for long lists of IRQHandlers, etc.


Re. the proposition to excise the dsy_gpio_pin from the library:

I also like this idea, and I think it would be a good step toward some much-needed house keeping throughout the library.
That said, I think we need a decent distribution of non-git based, pre-compiled binaries for older versions, with clear documentation for migration between versions before we can really dig in and clean up.

As a first step in that we are actively working on a documentation site for the entire Daisy Ecosystem (both hardware, and software). An initial version of that will likely be live in the next month or two.
Following that, we can compile, and host the existing releases in a way that is easy to access along with the changes, and release notes.
At that point, it will be a little bit easier to harmonize some of these "breaking" changes that will require refactors of people's existing projects to run, with the much needed changes to the library to keep it clean and able to grow in a maintainable way.

So with that said, I think for now let's aim to keep backwards compatibility with these changes, and then revisit some of the larger changes in the coming months.


In other news, we're a good chunk of the way through doing a validation test with the hardware on this PR. Only ones left to check are the Petal, Versio, and Legio.
Everything's working well with C++, oopsy (Max/MSP Gen~), and pd2dsy! :)

@stephenhensley
Copy link
Collaborator

@stellar-aria Alright! Testing completed.
We've confirmed everything tests okay on the hardware (except for Legio, but we did check over the pin changes carefully).

Aside from the remaining style changes, this is ready to merge. 🚀

@stellar-aria
Copy link
Contributor Author

@stephenhensley Completed!

@stephenhensley
Copy link
Collaborator

Okay!
We did a little bit more testing just to make sure everything checked out on the PatchSM dev board.
Everything checks out, and it's passed all of our tests on this end.

Merging now.
Thanks again for the contribution!

@stephenhensley stephenhensley merged commit e933241 into electro-smith:master Jun 13, 2023
@stellar-aria stellar-aria deleted the daisy_pin_migration branch June 14, 2023 00:11
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