Skip to content

Add basic support for Disting Ex dual mode commands#434

Merged
trentgill merged 4 commits intomonome:mainfrom
ryland:main
Nov 8, 2021
Merged

Add basic support for Disting Ex dual mode commands#434
trentgill merged 4 commits intomonome:mainfrom
ryland:main

Conversation

@ryland
Copy link
Copy Markdown
Contributor

@ryland ryland commented Sep 25, 2021

Finally had a chance to connect disting to crow, so I went ahead and added these commands from the 1.9 firmware.

Not sure if the names for dual mode should be abbreviated-- they are getting a bit long.

* Allow getters to use separate arguments for side and parameter, instead of forcing
the caller to pack them each time.
@tehn tehn requested a review from trentgill September 29, 2021 12:24
@tehn
Copy link
Copy Markdown
Member

tehn commented Sep 29, 2021

looks good--- don't have a disting so can't test but figure you have :)

@trentgill checking if you have opinions on name lengths

Copy link
Copy Markdown
Collaborator

@trentgill trentgill left a comment

Choose a reason for hiding this comment

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

all looks good to me overall. couple small inline thoughts & ideas, but it's in a good place already.

could you do a quick tab->spaces convert (there's a couple places where indentation gets off).

, cmd = 0x57
}
-- dual mode algorithms
, { name = 'set_dual_parameter'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think we can drop set_ as this is implicit in usage: ii.disting.dual_parameter(2, 23).

below, getting is already implicit in the call (not the fn name): ii.disting.get('dual_parameter', 2)

same comment for set_dual_scale_parameter

, { 'value', u16 }
}
}
, { name = 'load_dual_algorithm'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

consider if we can drop load in this case? there is no save_dual_algorithm, so it seems unnecessary.

same for load_dual_algorithms.

Comment on lines +235 to +236
data[1] = (data[1] & 0xF0) | (data[2] & 0xF);
data[1] = (data[1] & 0x0F) | ((side & 0xF) << 4);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i'm out of bit-manipulation practice, but couldn't this just be:

data[1] = ((data[1] & 0xF) << 4) | (data[2] & 0xF);

then you could delete the side variable.

if there's something trickier going on, perhaps a short inline comment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm rusty on manipulation as well... hence a step based approach, but this is much more concise.

@ryland
Copy link
Copy Markdown
Contributor Author

ryland commented Sep 29, 2021

All the comments are good/helpful. I'll clean everything up when I have time in the next day or or two. Thanks @trentgill

* Use pickle to pack side/param for dual_parameter commands.
* Remove set_ from function names.
@ryland
Copy link
Copy Markdown
Contributor Author

ryland commented Oct 6, 2021

@trentgill I've cleaned this up a bit and am now handling all commands that use the packed side/param in pickle().

I removed all set_ prefixes from command names in order to match convention, and removed load_ where there was no corresponding save_. I think this helped enough that I didn't do any other abbreviations.

@discohead this works for me in some quick tests, but maybe you have time to try it out to confirm or offer any suggestions?

@trentgill trentgill merged commit 5d60a89 into monome:main Nov 8, 2021
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.

4 participants