crayzeewulf / libserial

Serial Port Programming in C++
BSD 3-Clause "New" or "Revised" License
398 stars 141 forks source link

SerialPort.cpp: don't use high baudrates when not available #127

Closed ffontaine closed 5 years ago

ffontaine commented 5 years ago

On certain architectures (namely Sparc), the maximum baud rate exposed by the kernel headers is B2000000. Therefore, the current libserial code doesn't build for the Sparc and Sparc64 architectures due to this.

In order to address this problem, this patch tests the value of __MAX_BAUD. If it's higher than B2000000 then we assume we're on an architecture that supports all baud rates up to B4000000. Otherwise, we simply don't support the baud rates above B2000000.

Fixes build failures such as:

SerialPort.cpp: In member function 'int LibSerial::SerialPort::Implementation::GetBitRate(const LibSerial::BaudRate&) const': SerialPort.cpp:1226:14: error: 'BAUD_2000000' is not a member of 'LibSerial::BaudRate' case BaudRate::BAUD_2000000:

Fixes:

Signed-off-by: Fabrice Fontaine fontaine.fabrice@gmail.com

mcsauder commented 5 years ago

Hi @ffontaine , nice catch!

Your patch looks good to me and passes hardware tests. Would you mind making an addition to your PR for me? In following SerialPortConstants.h, (as I should have done initially), could you add checks for systems limited to B230400 as well?

The example is here. If you wouldn't mind, add #ifdef __linux__ and #endif /* __linux__ */ to SerialPort.cpp at about lines 1197 and 1247, respectively?

That should catch any remaining issues with the method you identified. Those changes as well as your current pass hardware bench testing.

mcsauder commented 5 years ago

On a side note, I hate that switch statement, but this Stack Overflow post should explain why it is there: https://stackoverflow.com/questions/47311500/how-to-efficiently-convert-baudrate-from-int-to-speed-t

mcsauder commented 5 years ago

In the chance that anyone else identifies a more elegant solution to GetBitRate(), here are the relevant values from termios.h from an Ubuntu 18.10 linux machine:

/* c_cflag bit meaning */
#ifdef __USE_MISC
# define CBAUD      0010017
#endif
#define  B0         0000000     /* hang up */
#define  B50        0000001
#define  B75        0000002
#define  B110       0000003
#define  B134       0000004
#define  B150       0000005
#define  B200       0000006
#define  B300       0000007
#define  B600       0000010
#define  B1200      0000011
#define  B1800      0000012
#define  B2400      0000013
#define  B4800      0000014
#define  B9600      0000015
#define  B19200     0000016
#define  B38400     0000017
#ifdef __USE_MISC
# define CBAUDEX    0010000
#endif
#define  B57600     0010001
#define  B115200    0010002
#define  B230400    0010003
#define  B460800    0010004
#define  B500000    0010005
#define  B576000    0010006
#define  B921600    0010007
#define  B1000000   0010010
#define  B1152000   0010011
#define  B1500000   0010012
#define  B2000000   0010013
#define  B2500000   0010014
#define  B3000000   0010015
#define  B3500000   0010016
#define  B4000000   0010017
#define __MAX_BAUD B4000000
#ifdef __USE_MISC
# define CIBAUD   002003600000      /* input baud rate (not used) */
# define CMSPAR   010000000000      /* mark or space (stick) parity */
# define CRTSCTS  020000000000      /* flow control */
#endif
mcsauder commented 5 years ago

@crayzeewulf , this PR looks good to me as-is, and better with the additional lines added. It is a good catch.

crayzeewulf commented 5 years ago

@mcsauder Thanks for checking into this. I am merging this pull request.

We may be able to replace that switch statement with a std::map<>. Something like:

const std::map<BaudRate, int> baud_rate_to_int = {
    {BaudRate::BAUD_50, 50},
    // ... blah...blah...
    {BaudRate::BAUD_4000000, 4000000} 
} ;

try {
    return baud_rate_to_int.at(baudRate) ;
} catch(const std::out_of_range&) {
    throw std::runtime_error(ERR_MSG_INVALID_BAUD_RATE) ;
}