-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat(behaviors): Add behavior metadata information. #2231
base: main
Are you sure you want to change the base?
feat(behaviors): Add behavior metadata information. #2231
Conversation
petejohanson
commented
Mar 27, 2024
- For upcoming ZMK studio work, make a set of rich metadata available to provide a friendly name for a behavior, and allow super flexible descriptions of the parameters the behaviors take.
55e55a0
to
05cb259
Compare
app/dts/behaviors/to_layer.dtsi
Outdated
@@ -9,6 +9,7 @@ | |||
/omit-if-no-ref/ to: to_layer { | |||
compatible = "zmk,behavior-to-layer"; | |||
#binding-cells = <1>; | |||
friendly-name = "Go To Layer"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't call this "Go To Layer" anywhere in the docs, just "To Layer". I think it'd be good to be consistent, do you prefer the former?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I was reading it.. I realized it was a pretty odd name. I'll update to match our current language.
app/dts/behaviors/toggle_layer.dtsi
Outdated
@@ -9,6 +9,7 @@ | |||
/omit-if-no-ref/ tog: toggle_layer { | |||
compatible = "zmk,behavior-toggle-layer"; | |||
#binding-cells = <1>; | |||
friendly-name = "Layer Toggle"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also typically called "Toggle Layer." Having a proper noun phrase rather than verb+action might be better in general, but I'd again tend to consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to consistency for now, but I do think this reads better than "Toggle Layer", IMHO.
05cb259
to
88042aa
Compare
app/src/behaviors/behavior_bt.c
Outdated
.position = 0, | ||
.value = BT_NXT_CMD, | ||
.friendly_name = "Next Profile", | ||
.type = BEHAVIOR_PARAMETER_VALUE_METADATA_TYPE_VALUE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think it would be easier to read these if the type came before the value/range/etc. Maybe this would be a good order?
- Position
- Name
- Type
- Value/range/etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Definitely reads better, thanks!
app/include/drivers/behavior.h
Outdated
#define ZMK_BEHAVIOR_REF_INITIALIZER(_dev) \ | ||
{ .device = _dev, } | ||
|
||
#define ZMK_BEHAVIOR_REF_DEFINE(name, ...) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the compiler complain about unused macro parameters? If not, you could replace the variable args with an explicit parameter list and then just pass in everything but ignore the unneeded parameters whenever metadata is disabled. As this is currently written, it's quite difficult to understand what parameters this takes in either case.
Also, an idea for reducing the number of macros that need alternate implementations:
struct zmk_behavior_metadata {
// Making this still defined but empty when metadata is disabled so we can write
// ".metadata = ..." and have it still compile when metadata is disabled.
#if IS_ENABLED(CONFIG_ZMK_BEHAVIOR_METADATA)
const char *name;
// any additional fields needed later go here...
#endif
};
#if IS_ENABLED(CONFIG_ZMK_BEHAVIOR_METADATA)
#define ZMK_BEHAVIOR_METADATA_INITIALIZER(name) \
{ .name = name }
#else
#define ZMK_BEHAVIOR_METADATA_INITIALIZER(name) {}
#endif
struct zmk_behavior_ref {
const struct device *device;
struct zmk_behavior_metadata metadata;
};
#define ZMK_BEHAVIOR_REF_INITIALIZER(dev, name) \
{ .device = dev, .metadata = ZMK_BEHAVIOR_METADATA_INITIALIZER(name) }
(Test for that idea: https://godbolt.org/z/GzaeT9dYh)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I dig it. Took a slightly different approach though and just passed the node_id into the macro, in case more gets added later, the ZMK_BEHAVIOR_METADATA_INITIALIZER
can handle that "internally".
Thoughts on the updated take?
e968c3a
to
0119f0b
Compare
fda849e
to
a9d724f
Compare
a9d724f
to
053cf08
Compare
app/include/drivers/behavior.h
Outdated
uint16_t max; | ||
} range; | ||
|
||
enum behavior_parameter_standard_domain standard; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With "standard" being an adjective here, would naming this domain
make more sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, since this is the union field that matches the type
field, I'd like the naming to match, which explains my choice here. Thoughts?
app/include/drivers/behavior.h
Outdated
BEHAVIOR_PARAMETER_STANDARD_DOMAIN_NULL = 0, | ||
BEHAVIOR_PARAMETER_STANDARD_DOMAIN_HID_USAGE = 1, | ||
BEHAVIOR_PARAMETER_STANDARD_DOMAIN_LAYER_INDEX = 2, | ||
BEHAVIOR_PARAMETER_STANDARD_DOMAIN_HSV = 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BEHAVIOR_PARAMETER_STANDARD_DOMAIN_HSV = 3, | |
BEHAVIOR_PARAMETER_STANDARD_DOMAIN_COLOR_HSV = 3, |
might make it clearer what HSV means. (If it isn't supposed to be a color, then this really needs to be clarified.)
Also, RGB seems like a generally more common format for sending color data. I guess we could add a *_COLOR_RGB value later, but then we'd have to deal with two different color formats everywhere.
One alternative would be to change the underglow code to store the value as RGB, then convert to HSV when loading. If the devicetree format needs to match, we could also change RGB_COLOR_HSB_VAL() to convert from HSV to RGB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had originally used "HSV" as the "native" format to make it easier to do increase/decrease brightness, IIRC. I don't have a strong opinion here, tbh. Maybe @Nicell has soe thoughts though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main concern is that you can only represent a small fraction of the possible RGB colors using integer HSV values. Assuming I correctly wrote the Python script I used to test this, rgb_underglow.c's hsb_to_rgb()
function can represent 2,189,687 out of 16,777,216 possible colors (about 13%).
struct { | ||
uint16_t min; | ||
uint16_t max; | ||
} range; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we are assuming that
- Nothing with a range of values will ever go above 65535.
- Values cannot be negative.
I'm not sure what types of values would use this, so I'm not sure if the first one is a safe assumption.
Given that you can write a negative number as a parameter in devicetree, then read it by casting to int32_t
in driver code, is limiting values to non-negative safe? Should we maybe have separate types for signed and unsigned ranges?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.... some good questions. I suppose for this, I was having a hard time imagining a range needing to be larger than this, or negative, but since we're trying to have this not have to change any time soon.... worth being a bit more flexible here.
Do we think { int32_t min; uint32_t max; } range
would work? Do we really think we need a range that's only negative numbers? or with a minimum above 2,147,483,647 ever?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixing signed and unsigned math in C is generally a great way to have a bad time. I'd be in favor of just using signed values for both min and max. I can't think of any real-world range of values that wouldn't fit into a signed 32-bit integer.
The first thing I could think of that might need large numbers is millisecond durations. With 16-bit, that could be a problem since you'd be limited to ~32 s signed or ~65 s unsigned. With signed 32-bit, you get ~25 days, which I can't imagine being insufficient for anything (if that's the kind of time scale your behavior is working on, you should probably be using seconds instead of milliseconds).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, totally fair. I'll update for signed 32-bit values. It'll increase the size a small bit, but not enough I think to warrant limiting things.
app/include/drivers/behavior.h
Outdated
struct { | ||
enum behavior_parameter_standard_domain param1; | ||
enum behavior_parameter_standard_domain param2; | ||
} standard; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding this correctly, this is effectively just a shortcut for
{
.type = BEHAVIOR_PARAMETER_METADATA_CUSTOM,
.custom = {
.sets_len = 1,
.sets = {{
.values_len = 2,
.values = {{
.position = 0,
.type = BEHAVIOR_PARAMETER_VALUE_METADATA_TYPE_STANDARD,
.standard = <param1>,
}, {
.position = 1,
.type = BEHAVIOR_PARAMETER_VALUE_METADATA_TYPE_STANDARD,
.standard = <param2>,
}},
}},
},
}
right? If so, that simplifies defining the data, but it adds a special case to the implementation that has to read this data. You also still have to use the more complex form for something like a MIDI note behavior that uses param1 = range[0, 127]
(note) and param2 = range[0, 127]
(velocity), since there's no way to represent a range type here.
Is there any way we could avoid the redundant ways to represent the same data? Maybe have some predefined constants and/or macros to let you build a metadata set that only has one item without all the boilerplate? Alternatively, could we support all possible value types in this inline form of writing the parameter data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... this is a very valid question..... After adding the necessary complexity for custom params.... do we really need both ways of doing this? I think I'm almost convinced we don't. Let me sleep on this, but if I can't come up with a compelling reason, I will refactor to "one single way of doing this" with some naming tweaks to make this saner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, pushed a major refactor here to simplify. Thoughts?
29be027
to
cd1c6d0
Compare
e5b4818
to
0598671
Compare
app/include/drivers/behavior.h
Outdated
}; | ||
|
||
enum { | ||
BEHAVIOR_PARAMETER_VALUE_METADATA_TYPE_VALUE = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need "metadata" here? This is the type of the parameter value, not the type of the metadata itself, so this naming could be a little confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shortened.
app/include/drivers/behavior.h
Outdated
if (api->get_parameter_metadata) { | ||
return api->get_parameter_metadata(behavior, param_metadata); | ||
} else if (api->parameter_metadata) { | ||
memcpy(param_metadata, api->parameter_metadata, sizeof(struct behavior_parameter_metadata)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor:
memcpy(param_metadata, api->parameter_metadata, sizeof(struct behavior_parameter_metadata)); | |
*param_metadata = api->parameter_metadata; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -209,9 +213,98 @@ static int on_macro_binding_released(struct zmk_behavior_binding *binding, | |||
return ZMK_BEHAVIOR_OPAQUE; | |||
} | |||
|
|||
#if IS_ENABLED(CONFIG_ZMK_BEHAVIOR_METADATA) | |||
|
|||
static int get_macro_parameter_metadata(const struct device *macro, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is massive. Any way it could be broken into smaller pieces?
The combination of loop conditions with param1_found/param2_found and continue/break statements is particularly difficult to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, cleaned this up a ton, after properly identifying the edges. Read better?
abf80c4
to
da6f7e4
Compare
* For upcoming ZMK studio work, make a set of rich metadata available to provide a friendly name for a behavior, and allow super flexible descriptions of the parameters the behaviors take. * Add ability to validate a zmk_behavior_binding against the behavior metadata available.
da6f7e4
to
3a92c3b
Compare