Skip to content

Fix for compatibility with other compilers/environments - #45

Closed
suzuryo3893 wants to merge 2 commits into
AutonomyLab:masterfrom
suzuryo3893:origin_dev
Closed

suzuryo3893 wants to merge 2 commits into
AutonomyLab:masterfrom
suzuryo3893:origin_dev

Conversation

@suzuryo3893

Copy link
Copy Markdown
Contributor
  • An initialization of a variable required. An undefined value results doing nothing in a switch-case statement.
  • Explicit settings for all options of serial communication including stop bits, character size, and parity are required.

Otherwize, it can be happened that Operation "7" (0x07) turns to be "135" (0x87)

@jacobperron jacobperron left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the patch!

I have a few nitpicks.

Comment thread src/serial.cpp
using namespace boost::asio;
port.open(portName);
port.set_option(serial_port::baud_rate(baud));
port.set_option(serial_port::baud_rate(baud));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick: use spaces instead of tabs

Comment thread src/serial_stream.cpp
namespace create {

SerialStream::SerialStream(boost::shared_ptr<Data> d, const uint8_t& header) : Serial(d), headerByte(header){
SerialStream::SerialStream(boost::shared_ptr<Data> d, const uint8_t& header) : Serial(d), headerByte(header), readState(READ_HEADER){

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick: Better to initialize readState before headerByte so they are in the order they are defined in the class.

@jacobperron jacobperron mentioned this pull request Aug 27, 2019
@jacobperron

Copy link
Copy Markdown
Member

I've addressed my own nitpicks in #50. Thanks again!

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