yujinrobot / yujin_ocs

Yujin Robot's open-source control libraries
http://wiki.ros.org/yujin_ocs
235 stars 178 forks source link

Do not recreate publisher/subscriber/timers if not needed on reconfig… #111

Closed corot closed 7 years ago

corot commented 7 years ago

…uration. With the typical configuration of 5 or 6 inputs, speeds-up reconfiguration like ~50-fold

As an extra, create another timer for cmd_vel messages from any source, so we can dislodge last active source if it gets stuck without further messages (sorry for the doubled PR)

AlexReimann commented 7 years ago

This adds c++11 requirements.

@stonier What's our current stand on that in public code?

stonier commented 7 years ago

Have to be careful about it with this package since it will force packages upstream to depend on c++11 as well (if c++11 symbols are exposed I think). Should not be folded in via the add_definitions either...even though it can, that is by convention for -D arguments.

stonier commented 7 years ago

Need to have a better gander at this when I get some time...didn't make it to it today. Have no idea what Jorge is up to :) Might be worth checking into the rotate bug we have at the same time too.

corot commented 7 years ago

Come on!!! C++11 rules 🤗

yes, there are std::shared_ptr in the header

Afaik, C++11 is highly recommended for Kinetic... I don't remember what the REP said exactly

So.... here it is: http://www.ros.org/reps/rep-0003.html So Daniel is right. We should avoid C++11 on exported headers... but this is a nodelet, not a lib, so should not be a problem

2016-09-20 12:41 GMT+02:00 Daniel Stonier notifications@github.com:

Need to have a better gander at this when I get some time...didn't make it to it today. Have no idea what Jorge is up to :) Might be worth checking into the rotate bug we have at the same time too.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yujinrobot/yujin_ocs/pull/111#issuecomment-248265655, or mute the thread https://github.com/notifications/unsubscribe-auth/AATsMv9XDO9tXnG2GUzWr9HjBNig_vOKks5qr7hFgaJpZM4KARJi .

stonier commented 7 years ago

Come on!!! C++11 rules

Aye, we use it everywhere for a long time (even arm core builds) except in open source libraries like the ecl. My rage is saved for gcc who will never include it as a default :) I think it will be default as c++14 in gcc 6.0 or something like that. That is a really really long time to wait for features that the majority are using, in product code, now.

but this is a nodelet

I forgot this point. No problems with it being c++11 as no-one will include/link to it. Perhaps just don't abuse the add_definitions to get it in.

corot commented 7 years ago

So... what's the preferred way? I'll modify the PR set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")

I saw add_definitions and pasted everywhere ..... :see_no_evil:

stonier commented 7 years ago

CHECK_CXXCOMPILER_FLAG and CMAKE_CXX_FLAGS is the preferred portable way (ecl_build has a cmake function for this).

In Cmake 3.1 there is a new target_compile_features which will be the more preferred way but it will take some time before that is a good default method. Probably we will have c++14 in g++ by default by then.

stonier commented 7 years ago

http://stackoverflow.com/questions/10851247/how-to-activate-c-11-in-cmake

stonier commented 7 years ago

@AlexReimann

Can you make a concise list of items still needed before merging and then merge when you're happy?

AlexReimann commented 7 years ago

Just this trailing \ is left I guess. Kind of fine with the other stuff :>

AlexReimann commented 7 years ago

I would squash the commits into one and then merge now.

@stonier Any black magic thing you want to do before that which maybe handles the divergence of kinect branch and our devel branch? :>

corot commented 7 years ago

Ups,,, sorry. Very busy on Friday and celebrating our reunification yesterday.

Squash your latest N commits into one git rebase -i HEAD~N and replace pick with squash where desired; need to push with -f (force) so... a bit dangerous!

bit-pirate commented 7 years ago

need to push with -f (force) so... a bit dangerous!

Come on, Jorge! Show some cojones! :-D

Just create your own fork. There you can play and mess up as you want, e.g. push -f. Later create a new PR from your fork.

stonier commented 7 years ago

Did this get PR'd, or cherry-picked across to devel?

AlexReimann commented 7 years ago

Nope

stonier commented 7 years ago

@AlexReimann can you make it so?

AlexReimann commented 7 years ago

Tested it and looks like no problems. Want me to do a PR or just merge it?

stonier commented 7 years ago

I usually manually cherry-pick and leave a note back in the PR here explaining and linking to the cherry-picked commit.

AlexReimann commented 7 years ago

Cherry picked to devel