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

Socket can #269

Merged
merged 10 commits into from Feb 20, 2017
Merged

Socket can #269

merged 10 commits into from Feb 20, 2017

Conversation

@destogl
Copy link
Contributor

destogl commented Mar 7, 2016

This is now full implementation of SocketCan abstraction interface. Code is tested against HW.

Code formatting is done in a way I understand the package is formatted (4 spaces).

@fmessmer

This comment has been minimized.

Copy link
Member

fmessmer commented Mar 7, 2016

This is following/finalizing #254, right?

@@ -13,6 +13,7 @@

<buildtool_depend>catkin</buildtool_depend>

<depend>cmake_modules</depend>

This comment has been minimized.

Copy link
@fmessmer

fmessmer Mar 7, 2016

Member

what do you need cmake_modules for?

This comment has been minimized.

Copy link
@fmessmer

fmessmer Mar 8, 2016

Member

see comment above!
Simply add <depend>boost</depend> instead of <depend>cmake_modules</depend>

@@ -1,7 +1,9 @@
cmake_minimum_required(VERSION 2.8.3)
project(cob_generic_can)

find_package(catkin REQUIRED COMPONENTS cob_utilities libntcan libpcan socketcan_interface)
find_package(catkin REQUIRED COMPONENTS cmake_modules cob_utilities libntcan libpcan socketcan_interface)

This comment has been minimized.

Copy link
@fmessmer

fmessmer Mar 7, 2016

Member

what do you need cmake_modules for?

This comment has been minimized.

Copy link
@destogl

destogl Mar 8, 2016

Author Contributor

Boost. Look at line 6.

This comment has been minimized.

Copy link
@fmessmer

fmessmer Mar 8, 2016

Member

You don't need cmake_modules for that. In fact, cmake_modules has no entries for Boost!

Simply add <depend>boost</depend> to the package.xml and find_package(Boost REQUIRED COMPONENTS thread) to CmakeLists.txt (nothing in find_package(catkin REQUIRED COMPONENTS ))

This comment has been minimized.

Copy link
@destogl

destogl Mar 8, 2016

Author Contributor

Oh :/ You are right. I mixed it in my head...

@destogl

This comment has been minimized.

Copy link
Contributor Author

destogl commented Mar 8, 2016

Exactly, it is finalizing #254

@destogl

This comment has been minimized.

Copy link
Contributor Author

destogl commented Mar 8, 2016

Travis is not happy! Any ideas?

…me to start the thread since this caused problems in some cases (on some computers).
@fmessmer

This comment has been minimized.

Copy link
Member

fmessmer commented Mar 8, 2016

Travis times out because there has not been any output on the console for about 10 mins....this is not particularly related to your PR...

Setting up python-catkin-pkg (0.2.10-1) ...

Setting up python-rosdistro (0.4.4-1) ...

Setting up python-rosdep (0.11.4-1) ...

Processing triggers for libc-bin (2.19-0ubuntu6.6) ...

sudo apt-get install -qq -y ros-${CI_ROS_DISTRO}-ros #needed as long as https://github.com/ros-infrastructure/rosdep/issues/430 is not fixed

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

The build has been terminated

@ipa-fmw can you have a look at the .travis config for cob_driver?

@fmessmer

This comment has been minimized.

Copy link
Member

fmessmer commented Mar 14, 2016

@ipa-mdl do you want to comment on the functionality added? (not the formatting)... 😉


static const int c_iInterrupt;
static const int c_iPort;
std::list<can::Frame> recived_frame;

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Mar 14, 2016

Member

received_frame

@ipa-mdl

This comment has been minimized.

Copy link
Member

ipa-mdl commented Mar 14, 2016

Is there a special reason for not using ThreadedSocketCANInterface (threading.h)?
And synchronous access could be implemented using BufferedReader (reader.h).

@destogl

This comment has been minimized.

Copy link
Contributor Author

destogl commented Mar 14, 2016

No, there is actually no reason, I didn't know they exist. I will change it in the next few days....

@destogl

This comment has been minimized.

Copy link
Contributor Author

destogl commented Mar 17, 2016

@ipa-mdl Thanks for the tip regarding ThreadedSocketCANInterface and BufferedReader.

ThreadedSocketCANInterface works perfectly, but I can not get BufferedReader to work. After some debugging I found out that handleFrame never gets called back.

Do you have any tips? In the new version of code you can see how I initalised the BufferedReader, for that I created new listen function with prototype:

void listen(boost::shared_ptr<CommInterface> interface)
@ipa-mdl

This comment has been minimized.

Copy link
Member

ipa-mdl commented Mar 17, 2016

Your code should not even compile. There is no BufferedReader::listen function with one argument. The current implementation needs a can::Header to be passed as a filter. I should have read the full post ;) Can you provide me your listen() implementation?
I have added a catch-all implementation for you here: ros-industrial/ros_canopen#173

@destogl

This comment has been minimized.

Copy link
Contributor Author

destogl commented Mar 18, 2016

My listen implementation is exactly the same as yours :)

void listen(boost::shared_ptr<CommInterface> interface){
        boost::mutex::scoped_lock lock(mutex_);
        listener_ = interface->createMsgListener(CommInterface::FrameDelegate(this, &BufferedReader::handleFrame));
        buffer_.clear();
    }

But still, this doesn't bring us closer to the problem solution... And really I have no idea why doesn't work, because the same code written in SocketCan.cpp file (see frame_listenter) work perfectly...

}
else
{
can::BufferedReader m_reader;

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Mar 18, 2016

Member

remove this line, it shadows the class member

@@ -3,14 +3,17 @@ project(cob_generic_can)

find_package(catkin REQUIRED COMPONENTS cob_utilities libntcan libpcan socketcan_interface)

find_package(Boost REQUIRED COMPONENTS chrono)

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Mar 18, 2016

Member

I will add the missing chrono dependency in socketcan_interface

This comment has been minimized.

Copy link
@destogl

destogl Mar 18, 2016

Author Contributor

Should I than remove it?

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Mar 18, 2016

Member

Yes, you can remove it from CMakeLists.txt, package.xml and the includes.

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 11, 2017

Member

socketcan_interface was patched in ros-industrial/ros_canopen@bc5e982

m_bInitialized = true;
bool bRet = true;
return true;
bool SocketCan::initCAN()

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Mar 18, 2016

Member

Is this function really needed?


if(frame.is_error){

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Mar 18, 2016

Member

If you want to print message while debugging you could include https://github.com/ros-industrial/ros_canopen/blob/indigo-devel/socketcan_interface/include/socketcan_interface/string.h, it contains the ostream operators.

@ipa-mdl

This comment has been minimized.

Copy link
Member

ipa-mdl commented Mar 18, 2016

All checks have failed

ros_canopen needs to be added to .travis.rosinstall for now, since the release version is outdated and does not contain the new header files.
In addition I suggest to not merge this PR until ros_canopen is ready for release. (https://github.com/ros-industrial/ros_canopen/milestones/Final%20indigo%20release)

@andreeatulbure

This comment has been minimized.

Copy link

andreeatulbure commented Mar 29, 2016

BufferedReader is working now. @destogl and I debugged the program and found out that we should have casted the ThreadedSocketCANInterface to CommInterface to make the BufferedReader work.

@fmessmer fmessmer mentioned this pull request Apr 1, 2016
@fmessmer

This comment has been minimized.

Copy link
Member

fmessmer commented Apr 1, 2016

We will hold this PR and follow @ipa-mdl's recommended strategy, i.e. merge after new release of ros_canopen....

@ipa-nhg

This comment has been minimized.

Copy link
Member

ipa-nhg commented Sep 13, 2016

@ipa-mdl is it ros_canopen released? could we merge this PR?

@ipa-mdl

This comment has been minimized.

Copy link
Member

ipa-mdl commented Jan 10, 2017

please restart travis job

@mgruhler

This comment has been minimized.

Copy link
Member

mgruhler commented Jan 11, 2017

@ipa-mdl done

Copy link
Member

ipa-mdl left a comment

Some minor updates needed.
Please rebase on indigo_dev afterwards.

pCMsg->setLength(recived_frame.dlc);
pCMsg->set(recived_frame.data[0], recived_frame.data[1], recived_frame.data[2], recived_frame.data[3],
recived_frame.data[4], recived_frame.data[5], recived_frame.data[6], recived_frame.data[7]);
if (m_reader.read(&frame, boost::chrono::seconds(1)))

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 11, 2017

Member

Please use nSecTimeout here instead of usleep

This comment has been minimized.

Copy link
@destogl

destogl Jan 17, 2017

Author Contributor

When I do this I get following error:

socketcan_interface/reader.h:92:75: error: no match for ‘operator+’ (operand types are ‘boost::chrono::steady_clock::time_point {aka boost::chrono::time_point<boost::chrono::steady_clock>}’ and ‘const int’)

The read function receives 'DurationType' which is what exactly?

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 17, 2017

Member

Any boost::chrono compatible duration.

if nSecTimeout means ns then boost::chrono::nanoseconds(nSecTimeout) should work.

@@ -13,8 +13,10 @@

<buildtool_depend>catkin</buildtool_depend>

<depend>boost</depend>

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 11, 2017

Member

not needed anymore

* You should have received a copy of the GNU Lesser General Public License
* along with this package. If not, see <http://www.gnu.org/licenses/>.
*****************************************************************************/

#include <cob_generic_can/SocketCan.h>
#include <boost/chrono.hpp>

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 11, 2017

Member

not needed anymore

std::cout << std::dec << std::endl;
recived_frame = frame;
recived = true;
bool SocketCan::initCAN()

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 11, 2017

Member

Not sure why this function is needed at all.

This comment has been minimized.

Copy link
@destogl

destogl Jan 11, 2017

Author Contributor

This is here bacause of compatibility with other implementations.

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 11, 2017

Member

this is no common interface, it is only used in CANPeakSysUSB.

This comment has been minimized.

Copy link
@destogl

destogl Jan 16, 2017

Author Contributor

Aha OK. I will remove then. I used CANPeakSysUSB as example :)

This comment has been minimized.

Copy link
@destogl

destogl Jan 17, 2017

Author Contributor

Oh yes now I know why I did it. Other nodes which are using this library are switching between CANPeakSysUSB and SocketCAN. That is the main reason to keep this. But I will take care to update them. This note could be also important for you too :)

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 17, 2017

Member

Which ndoes use it? And how? It is not part of the base class.

Copy link
Member

fmessmer left a comment

Also, indentation has changed...but that's minor and all I can review...

What about rebase and/or squash?

I'm taking a "good to merge" by @ipa-mdl for final approval

@@ -3,6 +3,7 @@ project(cob_generic_can)

find_package(catkin REQUIRED COMPONENTS cob_utilities libntcan libpcan socketcan_interface)


This comment has been minimized.

Copy link
@fmessmer
@@ -17,4 +17,5 @@
<depend>libntcan</depend>
<depend>libpcan</depend>
<depend>socketcan_interface</depend>

This comment has been minimized.

Copy link
@fmessmer
Copy link
Member

ipa-mdl left a comment

Just some minor clean-up needed :)

~SocketCan();
bool init_ret();
void init();
void destroy() {};

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 19, 2017

Member

unused -> remove

pCMsg->setLength(recived_frame.dlc);
pCMsg->set(recived_frame.data[0], recived_frame.data[1], recived_frame.data[2], recived_frame.data[3],
recived_frame.data[4], recived_frame.data[5], recived_frame.data[6], recived_frame.data[7]);
if (m_reader.read(&frame, boost::chrono::nanoseconds(nSecTimeout)))

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 19, 2017

Member

Please check against the other implementation, if nanoseconds are meant.
It would be nice, if you would add a comment to the base class to specify the correct unit.

const char* p_cDevice;
int m_iBaudrateVal;
bool m_bInitialized;
bool m_bSimuEnabled;

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 19, 2017

Member

unused -> remove

bool m_bInitialized;
bool m_bSimuEnabled;
const char* p_cDevice;
int m_iBaudrateVal;

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 19, 2017

Member

unused -> remove


bool recived;
can::Frame recived_frame;
static const int c_iInterrupt;

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 19, 2017

Member

unused -> remove

bool recived;
can::Frame recived_frame;
static const int c_iInterrupt;
static const int c_iPort;

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Jan 19, 2017

Member

unused -> remove

@destogl

This comment has been minimized.

Copy link
Contributor Author

destogl commented Feb 8, 2017

Should I squash commits?

@ipa-mdl

This comment has been minimized.

Copy link
Member

ipa-mdl commented Feb 8, 2017

Should I squash commits?

That would be great. Perhaps not to a single commit, but some logical groups.

@fmessmer

This comment has been minimized.

Copy link
Member

fmessmer commented Feb 16, 2017

@destogl will you squash soon?
Otherwise, I'd be ok merging this PR as is....as the commit messages are reasonable and not too many 😉

@ipa-mdl

This comment has been minimized.

Copy link
Member

ipa-mdl commented Feb 16, 2017

Otherwise, I'd be ok merging this PR as is....as the commit messages are reasonable and not too many

I would recommend to squash-merge it. The commits are mixed up.

@fmessmer

This comment has been minimized.

Copy link
Member

fmessmer commented Feb 16, 2017

@ipa-mdl

Perhaps not to a single commit, but some logical groups

Well squash-merge says

The 10 commits from this branch will be combined into one commit in the base branch

So you decide 😉

@ipa-mdl

This comment has been minimized.

Copy link
Member

ipa-mdl commented Feb 16, 2017

So you decide

1 commit is better than 10 commits with many internal fixes.
If @destogl has time for a clean-up, we can wair for it. Otherwise, we will squash-merge.

@fmessmer fmessmer merged commit 0df3282 into ipa320:indigo_dev Feb 20, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@destogl

This comment has been minimized.

Copy link
Contributor Author

destogl commented Feb 22, 2017

Sorry for not squashing, I was in vacation in lest weeks.

Thanks!

@destogl destogl deleted the iirob:socketCAN branch Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.