electro-smith / libDaisy

Hardware Library for the Daisy Audio Platform
https://www.electro-smith.com/daisy
MIT License
331 stars 141 forks source link

Update Daisy board definitions to use new Pin system #581

Closed stellar-aria closed 1 year ago

stellar-aria commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

stephenhensley commented 1 year ago

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.

stellar-aria commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@stephenhensley Completed!

stephenhensley commented 1 year ago

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!