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

Implement uart permission check in a more distro independent way #20

Open
wants to merge 1 commit into
base: OpenFIRE-dev
Choose a base branch
from

Conversation

bebu
Copy link

@bebu bebu commented Sep 20, 2024

This moves the permission check to after the devices have been discovered. It then checks the real needed group for the first device and checks if user running the process is in the group.

…rease compatability between qt versions 5 and 6
@SeongGino
Copy link
Member

Looks functionally good, just have a couple of concerns if you could check those out. 🙂

@bebu
Copy link
Author

bebu commented Sep 20, 2024

Sure! But I can't seem to find any comments?

@SeongGino
Copy link
Member

You should be able to see pending comments, can you not?
2024_09-21 213010

@bebu
Copy link
Author

bebu commented Sep 22, 2024

Actually not, its also not marked as being reviewed.

Copy link
Member

@SeongGino SeongGino left a comment

Choose a reason for hiding this comment

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

Just got back from a short break, and I forgot to actually send these as requested changes, apologies!

@@ -121,6 +128,23 @@ void guiWindow::PortsSearch()
if(!usbName.length()) {
PopupWindow("No OpenFIRE devices detected!", "Is the microcontroller board currently running OpenFIRE and is currently plugged in? Make sure it's connected and recognized by the PC.\n\nThis app will now close.", "ERROR", 4);
exit(1);
}else{
Copy link
Member

Choose a reason for hiding this comment

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

Weird to have an unresolved if branch that isn't inside the Unix conditional block? This should be bumped down to be part of if(getuid()) (and the closing brace should be outside of it too). Also consistent spacing.

}else{
#if !defined(Q_OS_MAC) && !defined(Q_OS_WIN)
if(getuid()) {
auto deviceName = usbName.first();
Copy link
Member

Choose a reason for hiding this comment

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

Question: does this actually work if we do not have any compatible devices plugged in? I was under the assumption that users outside of dialout don't get any devices (and we've already pruned out any system ttySX virt terminals at this point).

It would be ideal to inform the user first that they should have an ideal working user environment - which is how the original implem worked, then move to available device checks. Perhaps move this block to above/below checking the length of our available devices list?

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