Introduce HIDProperty
#48
Conversation
There was a problem hiding this comment.
Sorry, I am truly lost here and I can't follow you.
There are barely no commit message explaining the changes so I have to re-read all of the code to get what you are changing.
The PR itself has some explanations, but they need to be put in the commits themselves, not just in the PR message. This PR message will eventually get lost, when the commit descriptions aren't (especially if we switch to gitlab in the future).
It seems to me that the HIDProperty class is the equivalent of what I had in mind for the actuator, so I can't figure out the change in naming.
So instead of answering with words, I'll need from you a class diagram.
Which means, I'm going to block this PR until we get #27 sorted out, with an up to date class diagram. Then, this PR will need to have the matching changes in the diagram, and an explanation on how it is now used.
Sorry for being rude, but I am in the dark for understanding the high level logic of all of this, and I have a feeling you are too.
Edit: s/I am in the black/I am in the dark/
Right, sorry. I will try to fix that moving forward.
Well, I was not entirely sure what you had in mind for the HID Actuator. It seemed like this but I didn't know if this was exactly what you had in mind. HIDProperty seemed like a better name to me, but we can change it if you want.
I was planning to do that next, but yeah, it probably makes more sense to do it now. |
FFY00
force-pushed
the
hid-actuator
branch
6 times, most recently
from
e2b6898 to
95f7bc0
Compare
Introduces a HIDProperty class. A HID property represents a data type
from the HID report descriptor (eg. X, Y, etc axis, buttons, etc).
Each HIDProperty must implement a populate() method which receives a
high-level action and a HID event array. It will populate the HID event
array based on the high-level action.
Example:
(definition)
```python
class ExampleButtonProperty(HIDProperty):
...
def populate(action, packets):
for packet in packets:
if 'buttons' in action:
for key, val in action['buttons'].items():
setattr(packet, f'b{key}', 1 if val else 0)
```
(usage)
```python
...
action = {
...
'buttons': {
0: True,
1: False,
2: True,
}
...
}
packets = []
for _ in range(packet_count):
packets.append(EventData())
'''
endpoint.populate_hid_data() will call populate() for each HID property
in the endpoint, includes our populate() method defined above
'''
endpoint.populate_hid_data(action, packets)
for packet in packets:
endpoint.send(endpoint.create_report(packets))
```
Note: This is a simple example using buttons, there's not much to do
here, we just set the button value in the events. A example where more
things need to happen is in the axis. I didn't provide the axis as an
example because the code is a bit more involved and harder to read.
This should provide an overview on how the different components are
supposed to be used togather.
The idea is to define a HIDProperty for each HID usage and teach it how
to handle that data.
This patch just introduces the class and makes Endpoint to hold a list
the of HID properties. Integration with the rest of the codebase comes
in the next patches.
Signed-off-by: Filipe Laíns <lains@archlinux.org>
This patch introduces a new HIDProperty, AxisProperty, which knows how to handle the Axis Usages from the Generic Desktop Page. Signed-off-by: Filipe Laíns <lains@archlinux.org>
…rsion In a previous patch we introduced HIDProperty, classes which subclass it (eg. AxisProperty) should know how to handle specific HID usages. This patch replaces the current _simulate_action_* routines with HID properties. Instead of teaching Device how to handle every single HID usage we define HID properties. Each endpoint has a list of properties which is dynamically discovered from its report descriptor. When it's time to perform an action we generate the a HID event list (the lenght is based on the action duration and the device report rate) and ask each endpoint to populate. Signed-off-by: Filipe Laíns <lains@archlinux.org>
Signed-off-by: Filipe Laíns <lains@archlinux.org>
Signed-off-by: Filipe Laíns <lains@archlinux.org>
Signed-off-by: Filipe Laíns <lains@archlinux.org>
|
We now have docs and I've added descriptions to the commits. I think the descriptions I added should be enough but please let me know if there is something you can't follow. |
|
@bentiss ping |
|
I still don't get the separation between the I would really like you to try to see if the code is not cleaner if:
|
|
Before I make a proper reply I just want to clarify some things.
This means each
Do you mean that
This might seem a bit basic but I just want to make sure we are on the same page. |
Yes, you can have multiple (HID)Actuators.
I mean that the
|
Okay, the separation is simple.
The event data will be restructured, the current one is just a prototype. My idea was to use the actuators to entirely replace actions, for eg. when we are working with macros, a "press button 6" event would be replaced with a "move 50 dots on the X axis and press KEY_A down for 1sec" event.
If we bind them together we would end up with something like The HID actuators are also tied to the HID properties in the report descriptor (eg. logical max/min), so I think it makes more sense to have a list of them in each endpoint. I made a diagram, should make things much easier for you to visualize: https://gitlab.freedesktop.org/snippets/971 I think the depth of this diagram shows the design better than the other one.
Okay, the question to ask here is who needs to ask |
Signed-off-by: Filipe Laíns <lains@archlinux.org>
|
This pull request introduces 2 alerts when merging 7f63157 into 5930050 - view on LGTM.com new alerts:
|
|
@bentiss friendly ping, if you have time. |
|
OK, I'll try to not be too angry there, but I am.
On a PR, we work on the current code, not the future one. Either you have something, you submit it and we discuss, but I can not review something which is just in your head. That being said, nothing I told you prevent your idea to be working.
Yes, and?
I don't.
No, there is no question. I asked you to do one thing. I detailed it:
And all you are doing is not what I asked. So this is getting really tiring. Make use of subclassing to separate the concerns (the "action" from the HID conversion), and this should give a better and cleaner code. I am not asking something really complex, I am just asking for a change in how the design is. If you don't like my idea, I don't care. I want you to implement it nonetheless, to show me what it look like, and then we can decide which one is better. But spending countless time just bikeshedding over a design when every single decision needs to be justified is time definitively lost where we could have made a lot of progress. |
Replaces #45
This deals with the issues from #45 in a nicer way. Instead of a HID Actuator, like suggested, we introduce
HIDProperty. Each endpoint has a list of properties which is populated by parsing the report descriptor.HIDPropertyhas apopulatemethod which receives an action after it has been updated by the device actuators, and populates a list of HID events with the according HID data. This replaces theDevice._simulate_action_*.The idea is to subclass
HIDPropertyfor each supported HID data type (eg.AxisProperty).This allows us to drop
ActionTypeas everything is now dynamically resolved to HID properties.What do you think @bentiss? Does this resolve your concerns?