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

libshut.c riello_ser.c nut-scanner bits: banish non-standard u_char and u_short #866

Merged
merged 2 commits into from
Nov 13, 2020

Conversation

jimklimov
Copy link
Member

Probably standard C99 "uint8_t" and 16(?) bit short would be more proper,
especially for serial-line protocols. But the original code used types
derived from hardware implementation sized "char" and "short" - so they
stayed this way here.

Stems from efforts on #823 and #844 to get more clean and green system setups in CI. Due to the comment above, review would be welcome.

…and u_short

Probably standard C99 "uint8_t" and 16(?) bit short would be more proper,
especially for serial-line protocols. But the original code used types
derived from hardware implementation sized "char" and "short" - so they
stayed this way here.
@jimklimov jimklimov added the ready / code review Author (and CI) consider the PR worthy of human rewievers' time label Nov 10, 2020
Copy link
Member

@aquette aquette left a comment

Choose a reason for hiding this comment

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

Seems indeed good, with a 5 seconds review. Testable for non reg on any Eaton unit with a serial port and having USB HID (ie not older power ware with XCP)

Copy link
Member

@clepple clepple left a comment

Choose a reason for hiding this comment

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

LGTM

@jimklimov
Copy link
Member Author

Thanks @aquette for the suggestion, now I intend to test this on an OmniOS box that has Eaton 9PX 5000i UPS attached by USB cable as well as seen over SNMP :) Not sure how much the USB drivers would help with bug-hunt in serial port code, but at least similar variable type change in SNMP code is more auditable.

Not sure what to look out for, probably diff's of outputs from upsc with one or other same-named driver build.

@aquette
Copy link
Member

aquette commented Nov 12, 2020

testing through USB won't give anything, since libshut is the equivalent marshaling layer of libusb.
Then it's the common HID core above.
AFAIR, 9PX also have a serial port (RS 232 / DB9). Keep in mind that P2P ports are mutually exclusive.
So when testing RS232, remove the USB cord from the UPS ;)
hope it helps, I've no more ups@home, so can't help further

@jimklimov
Copy link
Member Author

In this case, my physical setup that is stranded in the now-inaccessible office won't be of much help.

I looked at systems headers I could get at, everywhere that has an u_char defined it is an unsigned char (same for shorts), so this PR does not change the outcome for those.

I suppose a further pedantic change to turn such arch-dependent cases into uint8_t and uint16_trespectively, would need more testing with real hardware.

@jimklimov jimklimov merged commit 3e46996 into networkupstools:master Nov 13, 2020
@jimklimov jimklimov deleted the fightwarn-u_char branch November 15, 2020 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready / code review Author (and CI) consider the PR worthy of human rewievers' time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants