Skip to content

Better port permission setup and errors #42

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

Jump to bottom
Merged
merged 4 commits into from
Aug 25, 2023
Merged

Better port permission setup and errors #42

merged 4 commits into from
Aug 25, 2023

Conversation

zachfeldman
Copy link
Contributor

This PR achieves two things:

  • Better error message for when you don't have the right permissions to connect to the LED matrix

Before:

zach@zach-laptopw:~/dev/inputmodule-rs (main)$ ./target/x86_64-unknown-linux-gnu/debug/inputmodule-control led-matrix --clock
Current Time = 22:30
thread 'main' panicked at 'Failed to open port: Error { kind: Io(PermissionDenied), description: "Permission denied" }', inputmodule-control/src/inputmodule.rs:392:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

After:

zach@zach-laptopw:~/dev/inputmodule-rs (main)$ ./target/x86_64-unknown-linux-gnu/debug/inputmodule-control led-matrix --clock
Current Time = 23:46
thread 'main' panicked at 'Permission denied, couldn't access input module port. Ensure that udev rule is installed or the port the module is on is otherwise accessible.', inputmodule-control/src/inputmodule.rs:398:80
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
  • A udev rule (per @kiram9 's suggestion) to give the right permissions to connect to the LED matrix by opening up its port and an addition to the README to make it clear to users how to install this rule (eventually we can make this part of an install script)

@zachfeldman zachfeldman requested a review from JohnAZoidberg June 1, 2023 04:08
JohnAZoidberg
JohnAZoidberg reviewed Jun 13, 2023
View reviewed changes
ledmatrix/README.md Outdated
1. Copy it to your udev rules directory. On Ubuntu, this is located at `/etc/udev/rules.d/`
2. Reload the rules with `sudo udevadm control --reload`

You can debug issues using the `udevadm monitor --environment` command. Ensure that the device's `idVendor` and `idProduct` match when you plug in the device.
Copy link
Member

Choose a reason for hiding this comment

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

I don't see udevadm monitor --environment showing anything when I plug in my device.
Do you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is interesting! I definitely am. Here's a gif and a log of what I see what I run this, plug it in, then unplug it:
udevadm-monitor--environment-run-jun-13-733am
udevadm-monitor--environment-run-jun-13-733am.log

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it seems to work now, hm.

zachfeldman
zachfeldman commented Jun 13, 2023
View reviewed changes
ledmatrix/50-framework.rules Outdated
# Framework Laptop 16 - LED Matrix Inputmodule
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice @JohnAZoidberg , thanks for these and the other changes! 👍

@zachfeldman zachfeldman force-pushed the better-port-permission-setup-and-errors branch from 9a55c4f to 247bc18 Compare August 11, 2023 13:57
@zachfeldman
Copy link
Contributor Author

@JohnAZoidberg just rebased this - could we merge it soon potentially?

@JohnAZoidberg JohnAZoidberg force-pushed the better-port-permission-setup-and-errors branch from 59a621f to 3c3eac6 Compare August 25, 2023 08:57
@JohnAZoidberg
Copy link
Member

Sorry for the long wait @zachfeldman. Thanks for the contribution! :D

@JohnAZoidberg
Copy link
Member

I tested it and adjusted a bit according to the best practices recommended by the arch wiki.
We don't need to open the permissions to 666. 660 is enough with udev giving access to the logged in user.

@JohnAZoidberg JohnAZoidberg merged commit 78ebefa into main Aug 25, 2023
@JohnAZoidberg JohnAZoidberg deleted the better-port-permission-setup-and-errors branch August 25, 2023 09:03
@zachfeldman
Copy link
Contributor Author

Hey no problem @JohnAZoidberg I know it's been a busy time for your team! Thanks so much for taking the time to review this/get it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnAZoidberg JohnAZoidberg Awaiting requested review from JohnAZoidberg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zachfeldman @JohnAZoidberg