Skip to content
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

Print HID Usages for scancodes when --print is used #45

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

EBNull
Copy link

@EBNull EBNull commented May 27, 2024

No description provided.

@KarsMulder
Copy link
Owner

I'm really on the fence about this one.

On one hand, providing information about MSC events is probably not going to hurt the user, and may help them understand why those events exist.

This feature feels like it belongs more to another program like evtest, but then again, I have been using evsieve as an evtest alternative myself. And just because evtest should have this feature doesn't mean that evsieve shouldn't.

However, evsieve calls itself a "reasonably lightweight" utility. "Reasonably lightweight" does not mean "minimalist". Features that few people use are acceptable as long as they have negligible performance impact on those who do not use the feature.

I imagine very few people map EV_MSC events with evsieve, and this pull request makes evsieve 240kb larger in release+strip mode, which is an increase of 24%. That impacts the load time of the program and how much memory it uses. I don't think many users would actually care about the scancodes, so it does feel somewhat bloaty.

Also, you may have noticed that Cargo.toml contains only two dependencies, both of which are associated with the rustlang team. I've been aggressively minimizing the dependencies of this program for reasons that I don't even know myself. This pull request tries to add a new obscure dependency, which means that I have to vet the dependency myself for security purposes. As well as all future versions of it. That sounds like work. It also pulls in thiserror as recursive dependency, which pulls in a bunch more of recursive dependencies. I know that thiserror is a bog standard library in the Rust ecosystem, but still.

I tried to implement the feature myself without the hut dependency (see commit 68b0fb0) in an attempt to reduce the bloat, but it still ended up increasing the size of the binary by 180kB, which is more than I expected. I think that there is a lot of overhead because each string pointer occupies an additional 16 bytes in addition to the actual data in the string.

I think that there are two ways to get this done with less bloat:

  1. Do not compile the HID tables into the binary, but instead ship a data file along with the application which gets loaded when the user actually prints EV_MSC codes.
  2. Find a much more compact way of encoding the HID tables. The actual zipped data should take up only about 16kB of space. The compressed data is embedded in the application, but decompressed only if the user actually needs it.

The first approach has the issue that it complicates the compilation and deployment; currently evsieve is a single binary and most users are still supposed to compile it themselves. The second approach has the issue that I'm going to need to add some decompression library to my dependencies.

Regardless of the approach, there is also the issue of how to display the usage tables in the print messages. It is said that 80 characters is the standard terminal width, and adding the usage information causes the prints to exceed that width. This causes line wrapping on small terminals, such as a terminals which occuppy half of my 16:9 screen. That causes line wrapping, and line wrapping is not pleasant to look at. I think that the additional inconvenience to thin terminals may not even be worth the extra information for most users.

I suppose I could print the information if a flag hidinfo is provided to the --print argument, so it is only displayed to those who really want to know. But then this feature loses the advantage of explaining to unknowning users why those MSC_SCAN events exist and what purpose they serve. Which makes the bloat even harder to justify.

My current thoughts are:

  1. I will not add hut as dependency of evsieve;
  2. I am undecided on whether to keep, revert, or optimize the manual implementation

@EBNull
Copy link
Author

EBNull commented May 31, 2024

Thanks for considering the proposal and working on a potential solution that meets the values of the project.

I understand your reasoning re dependencies and the like; you're right that this addition might not justify the addition of ~180kB.

I implemented this initially for myself and figured I could share what I had done.

Ultimately, the addition of this is of course your call - feel free to close this unimplemented. Thanks for your work on evsieve! I appreciate the thought that has gone into the program and the stream-based paradigm it operates under.

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.

2 participants