Skip to content

Conversation

@rewolff
Copy link
Contributor

@rewolff rewolff commented Dec 4, 2016

…uniform.

Fixes #1144

The return value of "close" is documented to be zero on success. So if (close (..)) works.
On error paths (except for ONE existing case) the close return value is not checked.

I've been naughty. In this commit I've also downgraded the "Device /dev/ttyUSB0 doesn't exist, so there's no point trying to acquire a lock" message in Serial.CPP from INFO to DEBUG. It clutters the output when running with -l 3, and nothing is happening.

What I would like to do too, as a cleanup is for where WIN32 is different create a
CLOSEHandle(h) define that evaluates to CloseHandle(toHandle (h)) on WIN32 and to close(h) on others. This removes 5 lines-of-code at a time, all over that source file..... Is my suggestion for the name of the define sensible (acceptable with C++ programming practises?)

@peternewman peternewman self-assigned this Dec 4, 2016
@peternewman peternewman added this to the 0.10.3 milestone Dec 4, 2016
Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Now actually leaving as a review:

Hi @rewolff thanks for this. Can you target our 0.10 branch, so we can get this into 0.10.3 potentially. Also you'll need to fix the lint issues, and the C++ build errors on Linux (probably due to not including errno.h from the looks of things).

@peternewman peternewman modified the milestones: 0.10.3, 0.10.4 Dec 6, 2016
@rewolff
Copy link
Contributor Author

rewolff commented Dec 6, 2016

Peter, I changed a lot of code, (albeit in a trivial way), that I dont' have a chance to test for myself. I'm not sure that pushing for an immediate release is a good idea. I am not familiar enough with git to know immediately what I need to do. So again this will have to wait for a "rainy sunday afternoon".

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

In the end I agreed with you and released anyway @rewolff .

You'll need to fix the lint issues, and the C++ build errors on Linux (probably due to not including errno.h from the looks of things) before we can merge this.

@peternewman peternewman modified the milestones: 0.11.0, 0.10.4 Apr 30, 2017
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.

Close in uartdmx is checked for return value...

2 participants