Skip to content

Introduce action_endpoint #45

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.

Sign up for GitHub

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

Open
FFY00 wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Introduce action_endpoint #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d304b1
device: introduce action_endpoints
FFY00 Mar 15, 2020
ba96790
device: get min/max axis value from the report descriptor
FFY00 Mar 15, 2020
2b79b8e
device: remove wrong button action
FFY00 Mar 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 37 additions & 13 deletions ratbag_emu/device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ def __init__(self, name: str, info: Tuple[int, int, int],
for i, r in enumerate(rdescs):
self.endpoints.append(Endpoint(self, r, i))

self._action_endpoint: Dict[ActionType, int] = {}
self._map_endpoint_actions()

self.report_rate = 100
self.fw = Firmware(self)
self.hw: Dict[str, HWComponent] = {}
Expand Down Expand Up @@ -75,6 +78,13 @@ def actuators(self, val: List[Actuator]) -> None:

self._actuators = val

def _map_endpoint_actions(self) -> None:
for i, endpoint in enumerate(self.endpoints):
for features in endpoint.parsed_rdesc.input_reports.values():
for feature in features:
if feature.usage_name in ['X', 'Y']:
self._action_endpoint[ActionType.XY] = i

def destroy(self) -> None:
for endpoint in self.endpoints:
endpoint.destroy()
Expand All @@ -96,23 +106,32 @@ def transform_action(self, data: Dict[str, Any]) -> Dict[str, Any]:

return hid_data

def send_hid_action(self, action: object) -> None:
def send_hid_action(self, endpoint_number: int, action: object) -> None:
'''
Sends a HID action

We assume there's only one endpoint for each type of action (mouse,
keyboard, button, etc.) so we send the action to all endpoints. The
endpoint will only act on the action if it supports it.

Comment thread
FFY00 marked this conversation as resolved.
:param action: HID action
:param endpoint_number: Number of the endpoint from where to send the action
:param action: HID action
'''
for endpoint in self.endpoints:
endpoint.send(endpoint.create_report(action))

def _simulate_action_xy(self, action: Dict[str, Any], packets: List[EventData], report_count: int) -> None:
# FIXME: Read max size from the report descriptor
axis_max = 127
axis_min = -127
endpoint = self.endpoints[endpoint_number]
endpoint.send(endpoint.create_report(action))

def _simulate_action_xy(self, action: Dict[str, Any], endpoint_number: int, packets: List[EventData],
report_count: int) -> None:
endpoint = self.endpoints[endpoint_number]
min = max = {}
for features in endpoint.parsed_rdesc.input_reports.values():
for feature in features:
if feature.usage_name in ['X', 'Y']:
axis = feature.usage_name.lower()
min[axis] = feature.logical_min
max[axis] = feature.logical_max

@bentiss bentiss Mar 18, 2020

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.

This should likely be provided by the actuator, which owns its own endpoint.

@FFY00 FFY00 Mar 18, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@bentiss bentiss Mar 23, 2020

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.

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).

@FFY00 FFY00 Mar 23, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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?

@FFY00 FFY00 Mar 23, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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).

@bentiss bentiss Mar 23, 2020

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.

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.

@FFY00 FFY00 Mar 23, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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_data is opaque to the actuator, but the caller Device knows 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.

@bentiss bentiss Mar 23, 2020

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 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.


assert 'x' in min and 'x' in max and 'y' in min and 'y' in max, 'Endpoint does not support XY movement'

# We assume a linear motion
dot_buffer = {}
Expand All @@ -139,6 +158,7 @@ def _simulate_action_xy(self, action: Dict[str, Any], packets: List[EventData],
dot_buffer = self.transform_action(action['data'])

for attr in ['x', 'y']:
axis_max = max[attr]
assert dot_buffer[attr] <= axis_max * report_count
step[attr] = dot_buffer[attr] / report_count

Expand All @@ -149,6 +169,8 @@ def _simulate_action_xy(self, action: Dict[str, Any], packets: List[EventData],
break

for attr in ['x', 'y']:
axis_min = min[attr]
axis_max = max[attr]
dot_buffer[attr] -= step[attr]
diff = int(round(real_dot_buffer[attr] - dot_buffer[attr]))
'''
Expand Down Expand Up @@ -180,6 +202,7 @@ def simulate_action(self, action: Dict[str, Any], type: int = None) -> None:
packets: List[EventData] = []

report_count = int(round(ms2s(action['duration']) * self.report_rate))
endpoint = self._action_endpoint[action['type']]

if not report_count:
report_count = 1
Expand All @@ -188,14 +211,15 @@ def simulate_action(self, action: Dict[str, Any], type: int = None) -> None:
packets.append(EventData())

if action['type'] == ActionType.XY:
self._simulate_action_xy(action, packets, report_count)
self._simulate_action_xy(action, endpoint, packets, report_count)
elif action['type'] == ActionType.BUTTON:
self._simulate_action_xy(action, packets, report_count)
pass

s = sched.scheduler(time.time, time.sleep)
next_time = 0.0
for packet in packets:
s.enter(next_time, 1, self.send_hid_action,
kwargs={'action': packet})
s.enter(next_time, 1, self.send_hid_action, kwargs={
'endpoint_number': endpoint,
'action': packet})
next_time += 1 / self.report_rate
s.run()
2 changes: 1 addition & 1 deletion tests/test_device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def test_mouse_report(self, device, event_data):

expected = EventData(x, y)

device.send_hid_action(expected)
device.send_hid_action(endpoint_number=0, action=expected)
time.sleep(0.1) # give time for the kernel to proccess all events

assert expected.x <= event_data.x <= expected.x
Expand Down