Add support for the Launchpad Mini (MK1)

[belak]

This adds support for the Launchpad Mini.

The device seems to be very similar to the Launchpad S and as such most of the code was copied from that implementation (and checked against the programmer docs), but missing a number of features, such as text scrolling.

There are a few things worth noting:

• I changed light_all_rapid to also take the DoubleBufferingBehavior as an argument. I personally think this is better than assuming DoubleBufferingBehaviod::None, as the Launchpad S implementation does - it may be worth changing that.
• The implementation of mini::Spec::flush (when copied from the Launchpad S) was incomplete, as it only set the main body LEDs. I updated it to send all the LEDs, which seems to be in line with the math (any time > 40 LEDs are updated, use rapid mode which lines up with all 80 LEDs divided by 2).
• Because the Mini, the Standard LP, and the LP S seem to share almost all the same implementation, it may be worth making this code generic or at least introducing a trait so it doesn't need to be copied between all the separate implementations. This would also make it so a subset of features on all 9x9 2-LED-color devices could pretty much be used interchangeably.

[belak]

I've been doing some experimenting, and it looks like the LP Mini uses exactly the same protocol as the Launchpad S. Including things like text scrolling, which isn't listed as supported in the programmer's guide.

It's also worth noting that all the devices I've tested (Mini and MK2) seem to support device_inquiry and version_inquiry, also including the LP Mini which doesn't list it in the programmer's guide either. The original LP might be the only device which supposedly doesn't support it?

[kangalio]

Very cool, thank you! I will have a look at the code in detail in an IDE later

I changed light_all_rapid to also take the DoubleBufferingBehavior as an argument. I personally think this is better than assuming DoubleBufferingBehaviod::None

Not sure why I did it that way, I'll get out my Launchpad S later and test if it works as expected with a DoubleBufferingBehavior parameter. If yes, it's probably better to add the parameter as you said

The implementation of mini::Spec::flush (when copied from the Launchpad S) was incomplete, as it only set the main body LEDs

Thank you, I'll double-check the implementation on a Launchpad S

Because the Mini, the Standard LP, and the LP S seem to share almost all the same implementation, it may be worth making this code generic

For sure, that would be great. It is unfortunate that I only own a Launchpad S out of those three, so I fear that significant refactors may introduce breakage in the Launchpad Mini backend which I cannot test.

It's also worth noting that all the devices I've tested (Mini and MK2) seem to support device_inquiry and version_inquiry, also including the LP Mini which doesn't list it in the programmer's guide either. The original LP might be the only device which supposedly doesn't support it?

Interesting. Perhaps someday there will be an opportunity to add support for the original Launchpad to this crate, at which point device inquiry and version inquiry can be tested

[kangalio]

I changed light_all_rapid to also take the DoubleBufferingBehavior as an argument. I personally think this is better than assuming DoubleBufferingBehaviod::None

Ah I think I know why there is no DoubleBufferingBehavior parameter on light_all_rapid: the light_* functions are supposed to be shorthands for the commono use cases of just setting a button to a color. The set_button_* functions are intended for advanced use cases, including double buffering. Perhaps there could be a new function set_all_buttons_rapid with support for double buffering?

[belak]

Ah I think I know why there is no DoubleBufferingBehavior parameter on light_all_rapid: the light_* functions are supposed to be shorthands for the common use cases of just setting a button to a color. The set_button_* functions are intended for advanced use cases, including double buffering. Perhaps there could be a new function set_all_buttons_rapid with support for double buffering?

That would work - it could also just be light_all (since it's the fastest way to do it on these devices and exposing the "rapid" seems like more of an implementation detail) and set_all_buttons_rapid (or similarly set_all_buttons).

I do think if those functions are meant to be quick shorthands that don't support double buffering, they should us DoubleBufferingBehavior::Copy as that's less surprising in the simple case (and all other shorthands already use that).

[belak]

The implementation of mini::Spec::flush (when copied from the Launchpad S) was incomplete, as it only set the main body LEDs

Thank you, I'll double-check the implementation on a Launchpad S

If you want, I'm happy to copy that code over - the main issue is that I don't have a Launchpad S to test with, just a Launchpad Mini (MK1) and a Launchpad MK2.

Because the Mini, the Standard LP, and the LP S seem to share almost all the same implementation, it may be worth making this code generic

For sure, that would be great. It is unfortunate that I only own a Launchpad S out of those three, so I fear that significant refactors may introduce breakage in the Launchpad Mini backend which I cannot test.

I'm happy to test any quick changes if you ping me.

It's also worth noting that all the devices I've tested (Mini and MK2) seem to support device_inquiry and version_inquiry, also including the LP Mini which doesn't list it in the programmer's guide either. The original LP might be the only device which supposedly doesn't support it?

Interesting. Perhaps someday there will be an opportunity to add support for the original Launchpad to this crate, at which point device inquiry and version inquiry can be tested

Another note: I've got another branch where I started playing around with refactoring some of the code and I fixed up the device/version inquiry code - there were a few parts where it seemed to not be as sure on the details. I'll probably submit a separate PR for that after this one gets in.

[belak]

Also, I'm sorry for all the random comments, but do you have any recommendations for how to structure shared code? It seems like the 81 button, 2-color launchpads all use the same protocol , but right now you have to have a separate implementation for each of them (due to how the code works which sets up the devices - it looks for the device name).

Also, I noticed your code style doesn't really match up with what rustfmt outputs - in particular tabs vs spaces and some code alignment. Is rustfmt alright to use, or should I disable it for this project and switch to tabs?

[kangalio]

That would work - it could also just be light_all (since it's the fastest way to do it on these devices and exposing the "rapid" seems like more of an implementation detail) and set_all_buttons_rapid (or similarly set_all_buttons).

I do think if those functions are meant to be quick shorthands that don't support double buffering, they should us DoubleBufferingBehavior::Copy as that's less surprising in the simple case (and all other shorthands already use that).

I agree light_all is probably better. My reasoning for light_all_rapid was that it's supposed to be obvious what MIDI messages are sent, because it's a low-level API and all. But putting an explanation in the method documentation should be enough.

I agree with using DoubleBufferingBehavior::Copy. To be honest I have no idea what I meant with the this allows for double buffering shenanigans comment on the DoubleBufferingBehavior::None.

If you want, I'm happy to copy that code over - the main issue is that I don't have a Launchpad S to test with, just a Launchpad Mini (MK1) and a Launchpad MK2.

No need, I already did (the Canvas implementation was indeed faulty and didn't update the side buttons)

I'm happy to test any quick changes if you ping me.

Thank you, that is really useful! :)

Also, I'm sorry for all the random comments, but do you have any recommendations for how to structure shared code? It seems like the 81 button, 2-color launchpads all use the same protocol , but right now you have to have a separate implementation for each of them (due to how the code works which sets up the devices - it looks for the device name).

I don't have recommendations yet. I want to look into it soon though

Also, I noticed your code style doesn't really match up with what rustfmt outputs - in particular tabs vs spaces and some code alignment. Is rustfmt alright to use, or should I disable it for this project and switch to tabs?

Thanks for being so considerate :) You can keep using rustfmt, since I've actually already reformatted the entire codebase to rustfmt on a separate branch anyways

[belak]

I just noticed you've been doing some work on the lp-mini branch in here and I'm pretty sure my comment wrapping I've been using isn't at the same width as yours. Because you seem to already be pulling in the primary changes in this branch, I'll rebase off of your lp-mini branch so there are less conflicts to deal with.

[kangalio]

Thanks for implementing this. Is the PR ready to be merged?

[belak]

Thanks for implementing this. Is the PR ready to be merged?

Yep, this should be ready. :)