Fixes for new warnings/behaviours in GCC7+/cppcheck. #480

mikepurvis · 2020-04-14T03:16:02Z

A bunch of stuff that came up when testing with the GCC9-based toolchain that will ship in Ubuntu Focal:

Add missing member initializations.
Add missing override specifiers.
Hacky cast for the 16 bit AVR float logic.

There are a few more member initializations to add in NodeHandle. Will get to those before merging.

Also, I believe this will break Bionic, and it's a pretty breaking change regardless, so this probably merits cutting a noetic-devel branch for it.

rosserial_client/src/ros_lib/ros/msg.h

romainreignier · 2020-04-14T19:33:42Z

About improving code quality, it would be nice to make the time related functions const like I did in our qt version.

Also, we could align the current toNsec with the ROS toNSec (capital S).
This is breaking the API but if a new noetic-devel branch is created, maybe it's the moment.

Regarding time functions, maybe we could try to use 64 bit variables even if I know that the major use case is on small microcontrollers so the time is not always an epoch time.
What do you think about that?

mikepurvis · 2020-04-14T21:33:48Z

PR is up for noetic-devel branch PR testing here: ros/rosdistro#24473

The tests will continue to fail until the Python 3 situation in rosserial_python is resolved. I might simply disable those tests in this PR so that it can merge and then open a separate one to get rosserial_python functional again.

Note also that the travis build will also have to be updated; there's no base VM for focal, so it should probably be switched to use the docker service, see: https://docs.travis-ci.com/user/docker/ (though that would be blocked on having the official ROS Noetic docker image— osrf/docker_images#395)

stertingen

Although I approve Python 3 support in general; I wonder why is it included in this PR (targeting C++warnings).

stertingen · 2020-04-16T17:27:07Z

rosserial_python/src/rosserial_python/SerialClient.py

@@ -545,7 +538,7 @@ def run(self):
 else:
 rospy.loginfo("wrong checksum for topic id and msg")

- except IOError as exc:
+ except ImportError as exc:


What could possibly raise an ImportError that is caught here?

I agree, this makes no sense. Reverting.

rosserial_python/src/rosserial_python/SerialClient.py

stertingen · 2020-04-16T17:31:57Z

rosserial_client/src/ros_lib/ros/node_handle.h

- uint8_t message_in[INPUT_SIZE];
- uint8_t message_out[OUTPUT_SIZE];
+ uint8_t message_in[INPUT_SIZE] = {0};
+ uint8_t message_out[OUTPUT_SIZE] = {0};

 Publisher * publishers[MAX_PUBLISHERS];
 Subscriber_ * subscribers[MAX_SUBSCRIBERS];


For the sake of consistency, publishers and subscribers should be initialized with = {nullptr}.

Fixed. I wonder why cppcheck complained about all the other ones and not these.

stertingen · 2020-04-16T17:39:06Z

rosserial_python/src/rosserial_python/SerialClient.py

+ try:
+ sent = self.socket.send(data[totalsent:])
+ except BrokenPipeError:
+ sent = 0


Why not raise RuntimeError(...) here?

The message "RosSerialServer.write() socket connection broken" below seems inappopriate for the case in which a BrokenPipeError was not caught, but zero bytes were sent anyway.

Hmm, yeah I wasn't sure what to do here. From https://stackoverflow.com/a/34920917/109517, it seems like socket.send can legitimately return zero in at least some non-error scenarios. I suppose the risk is that it would hard loop on this, especially if you had a non-blocking socket, but that's not the case here with a dedicated writer thread. Have updated it to ignore the 0 scenario and instead throw from inside the except.

mikepurvis · 2020-04-16T20:13:19Z

@stertingen Without proper Python 3 support landing for Noetic, the tests using rosserial_python don't pass. However, given the scope of the changes, perhaps it would make more sense to just disable those, merge the smaller original change here, and make a separate PR for fixing rosserial_python.

Notably: - Add missing member initializations. - Add missing override specifiers. - Hacky cast for the 16 bit AVR float logic.

stertingen · 2020-04-17T08:14:09Z

rosserial_client/src/ros_lib/ros/node_handle.h

- Publisher * publishers[MAX_PUBLISHERS];
- Subscriber_ * subscribers[MAX_SUBSCRIBERS];
+ Publisher * publishers[MAX_PUBLISHERS] = {nullptr};
+ Subscriber_ * subscribers[MAX_SUBSCRIBERS] {nullptr};


Suggested change

Subscriber_ * subscribers[MAX_SUBSCRIBERS] {nullptr};

Subscriber_ * subscribers[MAX_SUBSCRIBERS] = {nullptr};

mikepurvis · 2020-08-20T21:10:01Z

Closed in favour of #508.

mikepurvis commented Apr 14, 2020

View reviewed changes

rosserial_client/src/ros_lib/ros/msg.h Show resolved Hide resolved

mikepurvis changed the title ~~Fixes for new warnings/behaviours in GCC 7+.~~ Fixes for new warnings/behaviours in GCC7+/cppcheck. Apr 14, 2020

mikepurvis force-pushed the gcc7-fixes branch from e705cc7 to 8f803b3 Compare April 14, 2020 19:55

mikepurvis changed the base branch from melodic-devel to noetic-devel April 14, 2020 21:27

mikepurvis force-pushed the gcc7-fixes branch 2 times, most recently from a4a935b to d068dff Compare April 16, 2020 00:42

mikepurvis mentioned this pull request Apr 16, 2020

Python 3 compatibility for generate_client_ros_lib (test) #486

Closed

stertingen reviewed Apr 16, 2020

View reviewed changes

mikepurvis and others added 7 commits April 16, 2020 20:17

Fixes for new warnings/behaviours in GCC 7+.

01af2f4

Notably: - Add missing member initializations. - Add missing override specifiers. - Hacky cast for the 16 bit AVR float logic.

Resolve uninitMemberVar warnings in NodeHandle.

a9c9a97

Port of rosserial_python to py3.

f27ccfc

Generate the test client using PYTHON_EXECUTABLE.

33a1f32

Fixes and cleanup for rosserial_python py3 port.

5d1c5c2

Update pyserial rosdep.

5dd1f6f

Use nullptr consistently.

83a4e20

mikepurvis force-pushed the gcc7-fixes branch from d068dff to 83a4e20 Compare April 16, 2020 20:18

mikepurvis added 2 commits April 16, 2020 20:20

Review fixups.

11d824d

Throw from inside the BrokenPipeError.

960c05b

This was referenced Apr 17, 2020

Fix Travis for Noetic + Python 3 #490

Merged

Make rosserial Python 3 friendly #364

Closed

stertingen reviewed Apr 17, 2020

View reviewed changes

acxz mentioned this pull request May 22, 2020

[Feature Request] Python 3 Compatibility #471

Closed

3 tasks

mikepurvis mentioned this pull request May 26, 2020

Noetic release #499

Closed

theseankelly mentioned this pull request May 29, 2020

Updates for python3 compatibility #500

Closed

mikepurvis mentioned this pull request Jul 10, 2020

rosserial_python serial_node: No module named 'SerialClient' #504

Open

mikepurvis mentioned this pull request Aug 20, 2020

Feature/noetic python support #508

Merged

mikepurvis closed this Aug 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for new warnings/behaviours in GCC7+/cppcheck. #480

Fixes for new warnings/behaviours in GCC7+/cppcheck. #480

mikepurvis commented Apr 14, 2020

romainreignier commented Apr 14, 2020

mikepurvis commented Apr 14, 2020 •

edited

Loading

stertingen left a comment

stertingen Apr 16, 2020

mikepurvis Apr 16, 2020

stertingen Apr 16, 2020

mikepurvis Apr 16, 2020

mikepurvis Apr 16, 2020

stertingen Apr 16, 2020

mikepurvis Apr 16, 2020

mikepurvis commented Apr 16, 2020

stertingen Apr 17, 2020

mikepurvis commented Aug 20, 2020

	Subscriber_ * subscribers[MAX_SUBSCRIBERS] {nullptr};
	Subscriber_ * subscribers[MAX_SUBSCRIBERS] = {nullptr};

Fixes for new warnings/behaviours in GCC7+/cppcheck. #480

Fixes for new warnings/behaviours in GCC7+/cppcheck. #480

Conversation

mikepurvis commented Apr 14, 2020

romainreignier commented Apr 14, 2020

mikepurvis commented Apr 14, 2020 • edited Loading

stertingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepurvis commented Apr 16, 2020

Choose a reason for hiding this comment

mikepurvis commented Aug 20, 2020

mikepurvis commented Apr 14, 2020 •

edited

Loading