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

Feat/add oscc interface node #1

Merged
merged 37 commits into from
Nov 2, 2017
Merged

Feat/add oscc interface node #1

merged 37 commits into from
Nov 2, 2017

Conversation

rebpdx
Copy link
Contributor

@rebpdx rebpdx commented Sep 5, 2017

This pull request adds a ROS node which exposes the OSCC API in the form of ROS messages so that a ROS user may utilize OSCC in their node environment.

To build and run tests:
Make sure you have Kinetic installed: http://wiki.ros.org/kinetic/Installation/Ubuntu

mkdir -p catkin_ws/src
cd catkin_ws/src
git clone --recursive https://github.com/PolySync/roscco.git
git checkout feat/add-oscc-interface-node
cd ..
catkin_make -DKIA_SOUL=ON
catkin_make run_tests -DKIA_SOUL=ON

Prior to this commit this repository was bare. This commit adds the initial
interface for OSCC and ROS called roscco.
Prior to this commit the readme was lacking in information to a new user. This
commit fixes that by providing information about what ROSCCO is, how to build
it examples of using it and how to run the tests.
Prior to this commit the CMakefile and package.xml files had commented out
example code provided from the catkin_create which is not needed to run this
package. This commit fixes that by removing the commented out lines.
Jenkinsfile Outdated
echo 'Release Package Created!'
}
}
catch(Exception e) {

Choose a reason for hiding this comment

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

If you're just going to rethrow the exception, there's no point in having the catch block here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. I was a little skeptic about groovy allowing the catch to be skipped but you would be correct, it is most certainly allowed and exercised.

#undef min
#undef max

#include "rapidcheck/Seq.h"

Choose a reason for hiding this comment

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

Pardon my unfamiliarity with the local conventions, but why are we including all of rapidcheck's source in this git repo as opposed to linking a submodule? Seems like unnecessary noise in the source tree and a possible cause of maintenance-cost-inducing drift.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd agree the submodule option is far better. I was having difficulty with building it in the catkin_make but apparently was a bit dense that day as it works as a submodule now.

Prior to this commit this repo used a static library and the header files for
rapid check rather than a git submodule to work with ROS build system. This
commit removes the duplicate headers and static library in favor of the
submodule option while still working with the ros test process.
Prior to this commit there was a try catch finally block where the catch rethrew
the same error so that the finally would still clean resources despite failure.
This commit does away with the extra catch since it's possible to use a try
finally in groovy.
Prior to this commit other projects depending on roscco would fail due to the
roscco target library not being an artifact despite being listed as an output
target. This commit fixes that by listing the actual output libraries.
Prior to this commit the rosco_node file had inconsistencies with formatting.
This commit fixes that by updating the formatting to follow the rest of the
code.
Prior to this commit ROS initialization would intermittently throw ROS errors
not connecting to master but recover due to SIGIO interrupts from OSCC. This
commit fixes that by blocking interupts during the ROS initialization process.
Prior to this commit this repository did not follow the ROS style guide. This
commit changes that by using the clang-format file provied in the ROS style
guide instrucitons. http://wiki.ros.org/CppStyleGuide
Prior to this commit there were catkin linter errors. This commit fixes that by
addressing the errors produced.
Prior to this commit ROSCCO used an older version of OSCC this commit updates
the dependency.
Prior to this commit ROSCCO did not disable before closing the OSCC connection
relying on the 200 ms timoeout. This commit calls the disable before close to
avoid relying on the timeout upon close.
Prior to this commit the topics advertised were using CamelCase rather than
snake_case which is the ROS standard. This commit fixes that by changing the
advertised topics to snake_case.
Prior to this commit this repo did not have an example about how to use ROSCCO.
This commit fixes that by adding an example node that translates the ROS
joystick node messages into ROSCCO messages with documantation about how the
example works.
Prior to this commit there was a missing delete in one of the message
callbacks. This commit fixes that by adding the required delete function
to prevent memory leaks.
Prior to this commit the controller models were not called out and the steering
did not have any smoothing algorithm making it difficult to control. This commit
fixes that by calling out the game controllers used and adds an exponential
average to the steering control.
Prior to this commit it was required to include defines in CMakeLists.txt for
each available OSCC vehicle. OSCC now has a config file so that it does not
need to be tracked in each project depending on OSCC. This commit utilizes
that change.
Prior to this commit the joystick commander node example had non descript constants
and variables. This commit fixes that by placing the values in named variables and
adding comments to clarify what they are for and why.
@@ -1 +1,81 @@
# roscco

Choose a reason for hiding this comment

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

I'd prefer that the document structure be setup like...

Project Name

Overview

Stuff about what the point of this project is and what it's about

Getting Started

Dependencies

Building <whatever color-added text you'd like here>

Usage

Examples

API (if one exists)

Tests

Test Dependencies

Running Tests

License

Ye ol' MIT

Choose a reason for hiding this comment

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

The two most important parts of any open source project are the top-level headings... why it exists and what's its level of share-ittude.

README.md Outdated


## Building ROSCCO in a catkin project

Choose a reason for hiding this comment

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

I'm fairly certain that from this doc I wouldn't be able to set this thing up and actually run it on my machine... because I'm not sure where to start. I'm not sure what assumed knowledge or environment I'm supposed to already have, but it's such that running the build steps below on my machine produce some familiar errors.

So we're going to have to figure out how much is safe to assume about our user such that we can either disclaimer, "The following assumes that you already..." or provide more explicit setup instructions.

At least, as-is, I can't actually check this out and test it to approve the PR or not.

Choose a reason for hiding this comment

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

The same is true for the testing side.

enable_disable_pub_ = nh_.advertise<roscco::EnableDisable>("enable_disable", queue_size);

// Timed callback for publishing to OSCC < 200 ms
timer_ = nh_.createTimer(ros::Duration(0.05), &TeleopRoscco::timerCallback, this);

Choose a reason for hiding this comment

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

There are a variety of what are essentially constants throughout the code that should probably be hoisted. Like 0.05 and down on line #99, 0.99, and there are probably some more.

Choose a reason for hiding this comment

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

Also, unless they're supposed to be mutable we should set them up as consts whenever possible to ensure they're not mutated accidentally anywhere.

package.xml Outdated
<package>
<name>roscco</name>
<version>0.0.1</version>
<description>A OSCC to ROS interface</description>

Choose a reason for hiding this comment

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

An


int main(int argc, char** argv)
{
ros::init(argc, argv, "teleop_turtle");

Choose a reason for hiding this comment

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

teleop_turtle?

Copy link
Contributor Author

@rebpdx rebpdx Sep 25, 2017

Choose a reason for hiding this comment

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

I was reading the turtle joystick conversion node example as a baseline to start from for following ROS standards on how the community handles joystick messages.

Actually there is more lacking consistency when comparing to the next line teleop_roscco and the node name roscco_commander. Since ROS community uses teleop for Teleoperation I'll make sure the node name and respective variables reflect roscco_teleop for clarity and consistency.

@nathanaschbacher
Copy link

I could also use some architectural overview for how the interaction of timers/timeouts is supposed to work.

Prior to this commit the trigger threshold was a mutable value which could
yield undesirable results if the value was changed. This commit fixes that by
setting the variable to be a constant.
Prior to this commit ROSCCO depended on a development branch version of OSCC
rather than a released version of OSCC. This commit updates ROSCCO to depend
on the released version 1.1.0. It also updates the Fault Report message to
support the added dtcs value provided by the version upgrade.
Prior to this commit the documentation was lacking in clarity of what
dependencies were required prior to using the components in ROSCCO. Also the
README was lacking in clear header organizatoin. This commit fixes that by
reorganizing the README headers, providing more details about the dependencies
for this project and providing clarity of the Licensing information as well as
corrections to the grammer in the description of the package.
Prior to this commit the README documentation used old variable naming for the
example node. This commit fixes that by using the correct variable naming for
the ROSCCO example.
Prior to this commit the example changes made it into a previous commit but
were incomplete and contained dead code. This commit fixes that by removing
dead code and using the correct variable names.
Prior to this commit the mapping functions that were added for clarity were mapped
inversly to what their behavior are. This commit fixes that by mapping the triggers
and steering to the desired behavior.
Prior to this commit the example deviated from the ROS coding standards. This
commit fixes that by using the clang format to make it follow the ROS standard.
const double data_smoothing_factor = 0.1;
double steering_average_;

// Variable to ensure joystick triggers have been initialized_

Choose a reason for hiding this comment

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

Is the extra _ character at the end intentional?

ros::Subscriber joy_sub_;
ros::Timer timer_;

int previous_start_state_;

Choose a reason for hiding this comment

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

Is the initial value significant for these variables?

@@ -0,0 +1,3 @@
int32 FaultOriginBrake=0
int32 FaultOriginSteering=1
int32 FaultOriginThrottle=2

Choose a reason for hiding this comment

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

Is it a ROS convention to have enumerated types and alike be signed integers instead of unsigned?

README.md Outdated
```

Similarly to the OSCC API The Reports and CanFrame messages can be

Choose a reason for hiding this comment

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

Should there be a comma after API?

README.md Outdated

Similarly to the OSCC API The Reports and CanFrame messages can be
subscribed to as feedback about what is going on in OSCC. The Command and

Choose a reason for hiding this comment

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

Maybe this could read subscribed to in order to provide feedback?

README.md Outdated
EnableDisable messages will call the commands for the OSCC API.

OSCC requires that a message to each device is sent within a 200ms, ROSCCO
Copy link

@jlamb-at-polysync jlamb-at-polysync Oct 17, 2017

Choose a reason for hiding this comment

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

Maybe this could be:
OSCC modules will disable if no messages are received within 200 ms. It is recommended to publish messages to the OSCC modules at a rate of 20 Hz (every 50 ms).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about, "OSCC modules will disable if no messages are received within 200 ms. It is recommended to publish ROSCCO messages, at a rate of 20 Hz (every 50 ms) to ensure OSCC modules do not disable." I would like to make sure there isn't confusion that it's the person publishing the ROSCCO messages who need to be aware of this.

@@ -0,0 +1,6 @@
uint32 can_id
uint8 can_dlc
uint8 pad

Choose a reason for hiding this comment

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

Do we need these reserved/internal fields mapped to the SocketCAN can_frame structure definition (they don't exist on all platforms and are for internal usage typically)?

}

// Calculate the exponential average
double calc_exponential_average(double average, double setpoint, double factor)

Choose a reason for hiding this comment

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

Could these function parameters be constant?


// Smooth the steering to remove twitchy joystick movements
const double data_smoothing_factor = 0.1;
double steering_average_;

Choose a reason for hiding this comment

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

Is the initial value significant for this variable?

// Number of messages to retain when the message queue is full
const int queue_size = 10;

// Timed callback frequency set to meet OSCC < 200 ms messages requirement

Choose a reason for hiding this comment

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

Might be more clear to say OSCC recommends publishing at a rate of 20 Hz (50 ms == 0.05 s)

Prior to this commit the CanFrame message contained reserved values which may
or may not exist on other targets. This commit fixes that by removing them and
assigning the values on an individual variable basis.
Prior to this commit some variables were in an initializer list while other were
initialized in the class definition also there weren't const infront of function
arguments. This commit clarifies the example by placing const infront of the
function arguments and places all initilization values with the variable
decelaration.
Prior to this commit there was some confusing vocabulary. This commit fixes that
by restructuring some of the sentences.
Prior to this commit the FaultOriginId was a signed int for the enum which is
conflicting with the OSCC enum type. This commit fixes that by using unsigned
int.
@@ -1,6 +1,3 @@
uint32 can_id
uint8 can_dlc
uint8 pad
uint8 res0
uint8 res1
uint8[8] data
Copy link

@jlamb-at-polysync jlamb-at-polysync Oct 18, 2017

Choose a reason for hiding this comment

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

Is it possible to also define a constant for this size in the message definition file, just so you don't have to know its 8 bytes? SocketCAN uses CAN_MAX_DLEN for example in /usr/include/linux/can.h.

Something like this is what I had in mind:
const uint8 CAN_FRAME_DATA_MAX_SIZE = 8
...
uint8[CAN_FRAME_DATA_MAX_SIZE] data
...
for(int i = 0; i < CAN_FRAME_DATA_MAX_SIZE; i += 1)

Prior to this commit it was assumed the data message array size was known. This
commit fixes that by adding a constant to the CanFrameData msg file so that it
is clear what size the data array is.
Prior to this commit the repo was missing documentation for the public functions
and the constants did not have consistant naming. This commit fixes that by
using noisy snake case on constant variables across files and adds doxygen
comments to all public functions.
/**
* @brief RosToOscc class initializer
*
* This functoin constructs ROS subscribers which can publish messages to OSCC API.

Choose a reason for hiding this comment

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

Typo: functoin

/**
* @brief Cast OSCC report to ROS message and publish
*
* This function casts an oscc report type to a ros message and publishes the message onto ROS. Where the ROSMSGTYPE

Choose a reason for hiding this comment

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

This function description block has inconsistent capitalization of ROS and OSCC.

/**
* @brief Callback function to publish ROS SteeringCommand messages to OSCC.
*
* This function is a callback that consumes a ROS BrakeCommand message and publishes them to the OSCC API.

Choose a reason for hiding this comment

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

Should be ROS SteeringCommand

cotos added 2 commits November 2, 2017 12:43
Prior to this commit this repo had spelling errors and lower case acronyms. This
commit fixes that by correcting the spelling errors and making the acronyms
uppercase.
@cotos cotos merged commit 2a69cd5 into devel Nov 2, 2017
Copy link

@ZackPierce ZackPierce left a comment

Choose a reason for hiding this comment

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

👍

ROS_ERROR("Failed to block SIGIO");
}

topic_brake_report_ = public_nh->advertise<roscco::BrakeReport>("brake_report", 10);

Choose a reason for hiding this comment

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

Should probably use an explicatory constant, e.g. queue_size here, as below.

}

topic_brake_command_ =
public_nh->subscribe<roscco::BrakeCommand>("brake_command", 10, &RosToOscc::brakeCommandCallback, this);

Choose a reason for hiding this comment

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

Same story re: explaining arbitrary constant for queue_size.

int previous_back_state_ = 0;

// Number of messages to retain when the message queue is full
const int queue_size = 10;

Choose a reason for hiding this comment

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

The ROS style guide seems to suggest SHOUTY_SNAKE case for constants. http://wiki.ros.org/CppStyleGuide#Constants

template <class OSCCTYPE, class ROSTYPE, class ROSSUBTYPE>
static void cast_callback(OSCCTYPE* report, ros::Publisher* pub);

static void steering_callback(oscc_steering_report_s* report);

Choose a reason for hiding this comment

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

Distinctly lacking in commentary for public functions. Not a blocking concern for the current PR, but may be worth considering in the future.

@rebpdx rebpdx deleted the feat/add-oscc-interface-node branch November 3, 2017 23:38
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.

6 participants