Re-attempting rosserial for VEX Cortex #380

CanyonTurtle · 2018-07-05T19:19:58Z

Overview

The added package, rosserial_vex_cortex, allows the user to use rosserial on the VEX Cortex, ontop of the PROS kernel.

This is a re-attempt at #378, based on feedback from @romainreignier and @tfoote .
This time the added project is self-contained, has no unneeded PROS project files, and allows the user to generate PROS projects and install the rosserial libraries automatically (see README.md).

Limitations

Only tested on Linux 18.04LTS + ros melodic, relies on bash script.

tfoote

Overall this is much cleaner. I have a bunch of smaller style and cleanup suggestions. There's also a few places that look like they're ripe for adding/improving reuse. Those might be worth deferring for another PR.

Also depending on the thoughts of maintainers we could work in a branch in parallel with master to be less disruptive as we iterate on Vex support.

tfoote · 2018-07-06T07:02:54Z

rosserial_vex_cortex/README.md

+3. PROS - [install PROS toolchain](https://github.com/purduesigbots/pros/releases/tag/2.12.1)
+
+# Setup
+Stup a ROS workspace and build rosserial packages (including rosserial_vex_cortex) from source:


typo s/Stup/Setup/

tfoote · 2018-07-06T07:25:56Z

rosserial_vex_cortex/src/rosserial_vex_cortex/make_libraries.py

+rospack = rospkg.RosPack()
+
+# copy ros_lib stuff in
+rosserial_cortex_dir = rospack.get_path(THIS_PACKAGE)


It looks like this script is a duplicate of the one in rosserial_arduino. Except for the THIS_PACKAGE variable. It might be a good candidate for parameterizing and deduplicating in the future.

tfoote · 2018-07-06T07:26:25Z

rosserial_vex_cortex/src/rosserial_vex_cortex/make_libraries.py

+
+import rospkg
+import rosserial_client
+from rosserial_client.make_library import *


Importing * is a bad practice. I know it's done in the other version, but explicitly calling the things out make it much easier to debug.

tfoote · 2018-07-06T07:28:23Z

rosserial_vex_cortex/src/ros_lib/ros.h

+
+namespace ros
+{
+// #if defined(__AVR_ATmega8__) or defined(__AVR_ATmega168__)


You should clean out these commented values that are from the other verison.

Also above.

tfoote · 2018-07-06T07:29:43Z

rosserial_vex_cortex/src/rosserial_vex_cortex/SerialClient.py

@@ -0,0 +1,85 @@
+#!/usr/bin/env python


Can we not use this file from another instance/location? I don't see any local modifications.

tfoote · 2018-07-06T07:32:02Z

rosserial_vex_cortex/src/ros_lib/logger.h

+// 1 for yes
+#define DEBUG_MODE 0
+
+// apply a macro to all args of a variadic macro, used for logg


These old commented versions should be cleaned up.

tfoote · 2018-07-06T07:32:28Z

rosserial_vex_cortex/src/ros_lib/logger.h

+// will print to uart2. 
+// Use screen to access uart2 on the terminal with a serial-usb adapter+cable
+// on device USB0 at 115200 baud: screen /dev/ttyUSB0 115200
+#define loggy(fmtstr, ...) fprintf((uart2), "[%d]: "       \


The names of these functions should be cleaned up too.

tfoote · 2018-07-06T07:33:23Z

rosserial_vex_cortex/src/ros_lib/logger.h

@@ -0,0 +1,51 @@
+#ifndef __CANNON_LOGGER_H__
+#define __CANNON_LOGGER_H__


Include guards should be PACKAGE_NAME_FILE_NAME_H format. http://wiki.ros.org/CppStyleGuide

unnecessary parts

CanyonTurtle · 2018-07-06T21:25:19Z

@tfoote, Thanks for the suggestions. I have removed the unnecessary serial-node script from this subproject and cleaned up the function names, packages, and documentation.

tfoote

This looks much better. Just a few more comments.

tfoote · 2018-07-06T21:34:18Z

rosserial_vex_cortex/README.md

+# Requirements
+1. Linux (Only tested on Ubuntu 18.04LTS)
+2. ROS (Only tested on ROS Melodic) - [installation guide](http://wiki.ros.org/melodic/Installation/Source).
+3. PROS - [installation guide](tps://github.com/purduesigbots/pros/releases/tag/2.12.1)


Broken link and it says installation gude, but points to the tarball.

tfoote · 2018-07-06T21:47:31Z

rosserial_vex_cortex/src/ros_lib/helloworld.cpp

+#include <ros.h>
+#include <std_msgs/String.h>
+
+// run continuously by the setup function, publishes the time at 50hz.


This comment is out of date.

tfoote · 2018-07-06T21:49:22Z

rosserial_vex_cortex/src/ros_lib/logger.h

+
+#include "API.h"
+
+// Use screen to access uart2 on the terminal with a serial-usb adapter+cable


Documentation of the debug port would be good to add as a block in the README.

romainreignier

This new PR seems in a good shape to me.

Nonetheless, I have added a few minor comments, mostly cosmetic fixes in the README.

But I have a concern about the number of MAX_SUBSCRIBERS and MAX_PUBLISHERS. 2 seems very low to me.

romainreignier · 2018-07-07T08:57:09Z

rosserial_vex_cortex/CMakeLists.txt

+project(rosserial_vex_cortex)
+
+find_package(catkin REQUIRED COMPONENTS
+  message_generation


No message is generated so this dependency is not needed.

romainreignier · 2018-07-07T08:58:14Z

rosserial_vex_cortex/CMakeLists.txt

+# put the names of custom service files here
+# add_service_files(FILES
+# 
+# )


The same apply for this commented lines, they are not needed.

romainreignier · 2018-07-07T08:58:30Z

rosserial_vex_cortex/CMakeLists.txt

+# generate_messages()
+
+catkin_package(
+  CATKIN_DEPENDS message_runtime


And also this dependency is not used

romainreignier · 2018-07-07T08:59:21Z

rosserial_vex_cortex/README.md

@@ -0,0 +1,88 @@
+# rosserial for VEX Cortex
+
+This package contains everything needed allow you to run rosserial on the [VEX Cortex](https://www.vexrobotics.com/276-2194.html), on the [PROS Kernel](https://pros.cs.purdue.edu/cortex/index.html).


s/needed allow/needed to allow/

romainreignier · 2018-07-07T09:00:32Z

rosserial_vex_cortex/README.md

+catkin_make install # this will generate folders in the workspace that contain executable scripts. 
+```
+
+# Usage Example(Linux)


s/Example(Linux)/Example (Linux)/

romainreignier · 2018-07-07T09:09:15Z

rosserial_vex_cortex/README.md

+to view the UART 
+
+# Limitations
+Global scope causes segmentation faults. Do not try to use global scope. ROS-related objects in the global scope causes segmentation faults. The cause of this is still unknown, but it may have something to do with the implementation of the global memory segment.


The first and third sentences are quite similar (only "ROS-related objects" differ). Maybe a reworking would help to clarify.

Maybe something like:
Global scope variables causes segmentation faults. Unlike in the other rosserial examples, avoid ROS objects in the global scope.

romainreignier · 2018-07-07T09:09:52Z

rosserial_vex_cortex/package.xml

+  <run_depend>rospy</run_depend>
+  <run_depend>rosserial_msgs</run_depend>
+  <run_depend>rosserial_client</run_depend>
+  <run_depend>message_runtime</run_depend>


As mentioned earlier, does not seems useful.

romainreignier · 2018-07-07T09:11:39Z

rosserial_vex_cortex/src/ros_lib/CortexHardware.h

+    // any initialization code necessary to use the serial port
+    void init() {
+      // uart2 is for debugging, while stdout is initialized automatically.
+      usartInit(uart2, 115200, SERIAL_8N1);


Usually in rosserial, the default baudrate is 57600.

And the hardware class can offer a method to change it, see rosserial_arduino:

rosserial/rosserial_arduino/src/ros_lib/ArduinoHardware.h

Lines 88 to 90 in fe1e233

void setBaud(long baud){

this->baud_= baud;

}

romainreignier · 2018-07-07T09:14:24Z

rosserial_vex_cortex/src/ros_lib/helloworld.cpp

@@ -0,0 +1,43 @@
+/*
+ * rosserial Publisher Example for VEX Cortex


If possible, move the examples to a separate folder like in rosserial_arduino: https://github.com/ros-drivers/rosserial/tree/fe1e233f39d56931b5e223c5e53a2d9c2f9fb664/rosserial_arduino/src/ros_lib/examples

romainreignier · 2018-07-07T09:18:37Z

rosserial_vex_cortex/src/ros_lib/ros.h

+#include "CortexHardware.h"
+
+namespace ros {
+  typedef NodeHandle_<CortexHardware, 2, 2, 128, 128> NodeHandle;


The first two template arguments are the max number of subscribers and publishers;

rosserial/rosserial_client/src/ros_lib/ros/node_handle.h

Lines 99 to 104 in fe1e233

template<class Hardware,

int MAX_SUBSCRIBERS = 25,

int MAX_PUBLISHERS = 25,

int INPUT_SIZE = 512,

int OUTPUT_SIZE = 512>

class NodeHandle_ : public NodeHandleBase_

Are you really limiting the users of the default version of rosserial_vex_cortex to use a maximum of two subscribers and two publishers?

CanyonTurtle · 2018-07-09T16:36:58Z

@tfoote @romainreignier,

Thanks for the continued feedback. I was able to slim the package even more, and I fleshed out the documentation for setting up and configuring the serial connection, and touched it up overall. I also followed every suggestion here (except the setBaud hardware member function @romainreignier - I think the serial port must be configured in a special part of the VEX Cortex lifecycle, so I neglected this change. See init.cpp).

@romainreignier I also reset the publisher and subscriber counts to their defaults. The numbers being so low last time was leftover from my testing. Thanks for the find!

One topic of discussion: should I create a new branch for continued development of this package in coming weeks, or should I make future potential PRs on jade-devel?

romainreignier

About the branch, I am not sure, but if you don't touch any other rosserial packages, it should be ok to stay with jade-devel.

But for these kind of questions, maybe @mikepurvis can answer.

romainreignier · 2018-07-09T16:54:13Z

rosserial_vex_cortex/CHANGELOG.rst

@@ -1,6 +1,11 @@
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 Changelog for package rosserial_vex_cortex
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+0.1.1


I think that all rosserial packages should have the same version number so it should be 0.7.7. But I may be wrong. See with @mikepurvis

thanks, about to re-commit because my commit broke the building (oops).

CanyonTurtle · 2018-07-09T17:54:03Z

Oop, just re-added a couple files and dependencies, I broke the vex cortex build process with the last commit. This one is tested and working.

CanyonTurtle · 2018-07-10T16:49:40Z

@mikepurvis, what do you think of this PR? I think it is in good condition to accept. This version of rosserial_vex_cortex is self-contained and documented in-package, and has been closely reviewed by @tfoote and @romainreignier.

As I alluded to before, I intend to add further documentation and some additional examples to this package. For that purpose, do you think I should branch off, or stay on jade-devel?

CanyonTurtle · 2018-07-12T20:06:37Z

Note @mikepurvis : Right now, I am continually developing this package, but this version is stable. To facilitate continued development, I will leave this PR as is, for reviewing purposes, and I will branch off of my own fork for the continued changes I am working on for a separate PR later.

CanyonTurtle · 2018-07-24T21:13:34Z

@mikeferguson @mikepurvis,

Would you please tell me what you think of this additional hardware support? I am looking to get this through because I have commits that build off of this. I also have VEX V5 support, which is a brand-new platform that will be used by thousands of VEX Robotics teams in the coming few years.

mikepurvis · 2018-07-25T18:55:49Z

Looks great! Thanks to all the reviewers who weighed in, and sorry for the long delay.

mikepurvis · 2018-07-25T18:56:36Z

Changes have been squashed on merge, so you'll need to rebase your local work.

CanyonTurtle · 2018-07-25T20:13:30Z

@mikepurvis Thanks!! I will promptly rebase and follow up with more content - as I said, I have also ported the cutting-edge VEX V5 Robot Brain microcontroller, and I have tweaked this package slightly. Do you think it would be okay to put the new V5, and tweaks to this one, in one PR? or shall I split them up?

mikepurvis · 2018-07-25T20:30:34Z

I'm easy-going on that kind of thing. If you have a lot stuff and/or expect to be iterating on this over time, would it make sense to simply add you as a maintainer of the rosserial repo so you can merge your own changes upon review?

CanyonTurtle · 2018-07-25T20:38:31Z

@mikepurvis I would appreciate that :) thanks!

That would be convenient, especially because I am about to open a PR for V5/updated cortex work. Just rebased my fork's jade-devel as you said, and it is ready to go.

…am to jade-devel * commit 'a707373c7e9f9ad40ad9440550840e29960c9290': Changed hardcoded pin 13 to LED_BUILTIN (ros-drivers#328) Re-attempting rosserial for VEX Cortex (ros-drivers#380) Added service to force an Arduino hard reset in serial_node.py (ros-drivers#349) Fix compiling on boost > 1.66 (ros-drivers#362) Use the ! prefix introduced in gcc4mbed for mbed examples (ros-drivers#304) Add support for boolean parameters (ros-drivers#355) Change Travis to use clang-5.0.0 (ros-drivers#363) [python] fix an unboundlocalerror (ros-drivers#346) Added ESP32 support (ros-drivers#345) Retry opening the serial port every 3 seconds (ros-drivers#342) In rosserial_arduino, changed embedded type size for ROS uint64 to 8 bytes from 4. (ros-drivers#312) 0.7.7 Changes Copy src/examples to install dir so make_libraries.py doesn't fail (ros-drivers#336) Add overall spin timeout to rosserial read. (ros-drivers#334) Fixing formatting on files. (ros-drivers#333) Fix catkin lint errors (ros-drivers#296)

CanyonTurtle added 4 commits July 5, 2018 11:48

initial commit

00bc535

reorganized dir to provide project building script

748a17e

readme change

531f3fa

readme change

441b461

CanyonTurtle mentioned this pull request Jul 5, 2018

rosserial for VEX Cortex #378

Closed

tfoote reviewed Jul 6, 2018

View reviewed changes

CanyonTurtle added 4 commits July 6, 2018 13:27

removed more

daef54b

unnecessary parts

renamed logger functions to be more specific.

6a59c02

logger name changes

2ae4fc7

added to the readme

87d154b

tfoote reviewed Jul 6, 2018

View reviewed changes

CanyonTurtle added 2 commits July 6, 2018 15:34

readme diagram

b264691

image to specify uart configuration

b341506

tfoote approved these changes Jul 6, 2018

View reviewed changes

romainreignier suggested changes Jul 7, 2018

View reviewed changes

adjusted readme, documentation, and slimmed project structure

12c65d7

romainreignier reviewed Jul 9, 2018

View reviewed changes

re-added some necessary infrastructure

8df221c

romainreignier approved these changes Jul 10, 2018

View reviewed changes

CanyonTurtle added 2 commits July 10, 2018 13:53

Added documentation about how to prevent crashes.

a22ff27

added demos for using a joystick or a phone to control robot

da35a34

mikepurvis merged commit 86e262f into ros-drivers:jade-devel Jul 25, 2018

CanyonTurtle mentioned this pull request Aug 26, 2018

VEX Cortex Usage improvements and VEX V5 Support #385

Merged

		@@ -0,0 +1,51 @@
		#ifndef __CANNON_LOGGER_H__
		#define __CANNON_LOGGER_H__


		#include "API.h"

		// Use screen to access uart2 on the terminal with a serial-usb adapter+cable

		@@ -0,0 +1,88 @@
		# rosserial for VEX Cortex

		This package contains everything needed allow you to run rosserial on the [VEX Cortex](https://www.vexrobotics.com/276-2194.html), on the [PROS Kernel](https://pros.cs.purdue.edu/cortex/index.html).

		@@ -0,0 +1,43 @@
		/*
		* rosserial Publisher Example for VEX Cortex

	template<class Hardware,
	int MAX_SUBSCRIBERS = 25,
	int MAX_PUBLISHERS = 25,
	int INPUT_SIZE = 512,
	int OUTPUT_SIZE = 512>
	class NodeHandle_ : public NodeHandleBase_

Re-attempting rosserial for VEX Cortex #380

Re-attempting rosserial for VEX Cortex #380

Conversation

CanyonTurtle commented Jul 5, 2018

Overview

Limitations

tfoote 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

Choose a reason for hiding this comment

CanyonTurtle commented Jul 6, 2018

tfoote 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

romainreignier left a comment

Choose a reason for hiding this comment

romainreignier Jul 7, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanyonTurtle commented Jul 9, 2018 • edited Loading

romainreignier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanyonTurtle commented Jul 9, 2018

CanyonTurtle commented Jul 10, 2018 • edited Loading

CanyonTurtle commented Jul 12, 2018

CanyonTurtle commented Jul 24, 2018

mikepurvis commented Jul 25, 2018

mikepurvis commented Jul 25, 2018

CanyonTurtle commented Jul 25, 2018 • edited Loading

mikepurvis commented Jul 25, 2018

CanyonTurtle commented Jul 25, 2018 • edited Loading

romainreignier Jul 7, 2018 •

edited

Loading

CanyonTurtle commented Jul 9, 2018 •

edited

Loading

CanyonTurtle commented Jul 10, 2018 •

edited

Loading

CanyonTurtle commented Jul 25, 2018 •

edited

Loading

CanyonTurtle commented Jul 25, 2018 •

edited

Loading