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

Fixes issue #15: Return an error if the unix user does not have the a… #17

Merged
merged 1 commit into from
Apr 19, 2016

Conversation

julienw
Copy link
Collaborator

@julienw julienw commented Apr 14, 2016

…ppropriate right to open the device

@julienw
Copy link
Collaborator Author

julienw commented Apr 14, 2016

I have yet to test on foxbox. But it works fine on the example project.

@julienw
Copy link
Collaborator Author

julienw commented Apr 14, 2016

Tested on foxbox, this works well.

It's a fatal error ATM. I thought that if the device is present but is not readable, it should be a fatal error. Please tell me if you think otherwise (for example I can see that some computer would have /dev/ttyUSB0 etc for another reason) and in that case I can change the adapter to make it non fatal and write log instead.

@dhylands r?

@dhylands
Copy link
Contributor

I definitely think that it should be non-fatal. Otherwise, if something bizarre happened, then the program fails to startup and the user is totally in the dark about why their foxbox isn't working at all. If its non-fatal at least the user can find out through the UI, which they can't do if the program fails to start.

If the user (most likely a developer in this case) has other USB serial devices plugged in (I often do), then /dev/ttyUSB0 might be one of those devices and /dev/ttyUSB3 might be the zwave device. Things will get more interesting when we need to support both ZigBee and ZWave devices.

The ultimate solution will be to check the USB VID and PID associated with the serial port and then use that to choose the correct port, but I digress.

In the long run, we should probably not just write logs, but maintain some type of state so that the UI can figure out that there was a problem.

r+

@julienw
Copy link
Collaborator Author

julienw commented Apr 14, 2016

Otherwise, if something bizarre happened, then the program fails to startup and the user is totally in the dark about why their foxbox isn't working at all.

Not really in the dark as it's quite clear in the panic message about what happened: the /dev/XXX file is written, with the io::Error "Permission denied". Here is an example from foxbox:

thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: OpenzwaveError(CannotReadDevice("/dev/ttyUSB0", Error { repr: Os { code: 13, message: "Permission denied" } }))', ../src/libcore/result.rs:746

But I'll still make it non-fatal: I'll prepare another PR for the adapter tomorrow.

@dhylands
Copy link
Contributor

dhylands commented Apr 14, 2016

Where is the end-user going to see the "paniced" message?

The only way they have of seeing any messages at all is by using the web browser, but the foxbox isn't running, so thay can't see anything.

@julienw
Copy link
Collaborator Author

julienw commented Apr 15, 2016

At the launch time only (when the adapter starts up). The whole foxbox will panic actually.

@julienw
Copy link
Collaborator Author

julienw commented Apr 15, 2016

Note this is how we handle Results when starting adapters: https://github.com/fxbox/foxbox/blob/998b6e0e1e1fd3cba3b711783eaa7000c2f66a0d/src/adapters/mod.rs#L61-L69. This is where we should not panic and store the log instead.

I think that in the long run the adapters could return:

  • ok(): everything worked fine
  • Error::RecoverableError(cause): there was an error but this is not a good reason to panic
  • Error::FatalError(cause): this is really serious, we should panic (but the caller can decide otherwise)

But I should fire up an RFC for this to be discussed, likely there are other/better solutions.

@julienw
Copy link
Collaborator Author

julienw commented Apr 15, 2016

see fxbox/RFC#14 for the RFC about errors

@julienw
Copy link
Collaborator Author

julienw commented Apr 15, 2016

On one of my computer I actually have a lot of /dev/ttyACMxxx devices so this convinced me that this error should not be a fatal error.

@julienw julienw merged commit e896187 into fxbox:master Apr 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants