Introduce action_endpoint
#45
Conversation
There was a problem hiding this comment.
Not convinced by the way the action_endpoint is defined/used.
Either you need to use the type, and thus you can infer that information from the report descriptors, either you can't and so you probably just need the endpoint number, and you don't need to store the map endpoint/action
| if feature.usage_name in ['X', 'Y']: | ||
| axis = feature.usage_name.lower() | ||
| min[axis] = feature.logical_min | ||
| max[axis] = feature.logical_max |
There was a problem hiding this comment.
This should likely be provided by the actuator, which owns its own endpoint.
There was a problem hiding this comment.
The actuator does not own an endpoint. The device owns the endpoints and just uses actuators to transform data before sending.
I think we should keep actuators simple, they just represent a hardware property and transform data accordingly. They should know nothing about the HID protocol.
There was a problem hiding this comment.
The actuator should still have its own min/max. This is a hardware capability, so there is nothing wrong to have this information in the actuator itself.
How you fetch the information (from the endpoint/HID report descriptor) is orthogonal to the fact that you should store the information in the actuator (once and for all, when you create your device).
There was a problem hiding this comment.
But the min/max is only in place at the HID level. The actuator translates units in high-level actions (eg. 5mm = 5000 dots). I could move my mouse for 1 mile if I wanted to.
Where the limits appear is in the high-level action to HID events translation. Given a certain number of reports and a certain logical max/min per report, a limit appears.
I guess this could be simplified by saying that the min/max is HID property, not a sensor property.
If we had a sensor that was only able to consecutively read a certain distance (eg. can only detect movement up to 30mm, after that you would need to stop the mouse and start the action again), then we would implement a maximum value in the actuator that represents that sensor. Does that make sense?
There was a problem hiding this comment.
One example where setting the min/max in the actuator doesn't work would be a weird device that reports axis movement on one of two endpoints with different min/max (it would select the endpoint by either having some configuration or based on context).
There was a problem hiding this comment.
If we had a sensor that was only able to consecutively read a certain distance (eg. can only detect movement up to 30mm, after that you would need to stop the mouse and start the action again), then we would implement a maximum value in the actuator that represents that sensor. Does that make sense?
well, a sensor is supposed to represent the physical chip. So at some point, your chip would have a hard limit in what values it can give you, being number of bits or the accuracy.
One example where setting the min/max in the actuator doesn't work would be a weird device that reports axis movement on one of two endpoints with different min/max (it would select the endpoint by either having some configuration or based on context).
Well, here, you need 2 actuators with different min/max.
This is often the case with Wacom devices: depending on the configuration you have 2 endpoints for the same stylus. Then you also have one actuator for the finger processing, so that would make 3.
Actuators are cheap. My main concern here is that you are basically parsing the report descriptor for every call to _simulate_action_xy(), when you should cache this value somewhere. And the actuator seem like a good place to store. In the same way, it seems to be a good place to store the associated data.
If you really don't want to have a HID actuator, you can always add the associated endpoint and some HID data in an object, and store this object in an opaque field device_data. device_data is opaque to the actuator, but the caller Device knows how to use it.
There was a problem hiding this comment.
Actuators are cheap. My main concern here is that you are basically parsing the report descriptor for every call to
_simulate_action_xy(), when you should cache this value somewhere. And the actuator seem like a good place to store. In the same way, it seems to be a good place to store the associated data.
I was not aware the report descriptor was parsed when accessing these fields.
Risking getting a bit off-topic here, is there any reason for this? Can the report descriptors change over time without us being notified? If not, maybe hid-tools should be caching it?
If you really don't want to have a HID actuator, you can always add the associated endpoint and some HID data in an object, and store this object in an opaque field
device_data.device_datais opaque to the actuator, but the callerDeviceknows how to use it.
I would prefer that approach, yes.
Storing the min/max in the actuator would also mean that we need to change the way we are processing the actuators right now. We would query them on each HID event instead of each high-level action. We can generate thousands of HID events, so I don't know if we are maybe better off with just parsing the report descriptor.
There was a problem hiding this comment.
I was not aware the report descriptor was parsed when accessing these fields.
We are not technically parsing the report descriptor, the code is looping through all of the endpoints, then all of the reports, then all of the fields, and then accessing the value.
So from a high level point of view your code is "parsing" the report descriptor, because it tries to make sense of it.
If you store the value in, let's say the actuator, the access time is 1, instead of n, because you don't need to check all the fields in the report descriptor.
Storing the min/max in the actuator would also mean that we need to change the way we are processing the actuators right now. We would query them on each HID event instead of each high-level action. We can generate thousands of HID events, so I don't know if we are maybe better off with just parsing the report descriptor.
Sigh. You are really making things confusing for yourself.
You are mixing up how you are supposed to access the information, and the technical detail code implementation.
If you want to have a HID_actuator, then all you need to have is one per report. Each of them would be called once per high-level action, but each would tell you which endpoint it would emit its events too. Then you filter out which you want.
The only difference with right now, is that you will have maybe more actuators, in the case you have similar reports. But you shouldn't have a difference in code.
Also, a proper way is to say that you have your current actuator, but you subclass it as a HID one.
This way, when you need a "clean" actuator, you use the actuator interface (not in the ML sense, in the API way), but the code itself will instantiate a HID one, and will use its data when it needs. It is no further different than the device_data I gave you, except that it is far superior in the object world.
FFY00
force-pushed
the
action-endpoint
branch
2 times, most recently
from
4076918 to
b00270c
Compare
action_endpoints is used to specify which endpoint performs each type of action. Signed-off-by: Filipe Laíns <[email protected]>
Signed-off-by: Filipe Laíns <[email protected]>
Signed-off-by: Filipe Laíns <[email protected]>
No description provided.