ethz-asl / cad-percept

Bringing meshes to robotics.
BSD 3-Clause "New" or "Revised" License
5 stars 1 forks source link

Feature/utils #35

Closed michaelpantic closed 5 years ago

michaelpantic commented 5 years ago

PR that contains a bunch of utilities/plumbing things:

Resulting statistics output looks something like this:

~~~~~~~~~~~~~~~~~~~~~~~~~~ Timings  ~~~~~~~~~~~~~~~~~~~~~~~~~~
Name                                  Count        Avg [us]        Min [us]        Max [us]
VertexNormals                            30    1.270000e+02    9.000000e+01    4.020000e+02
ethzasl-jenkins commented 5 years ago

Test PASSed.

ethzasl-jenkins commented 5 years ago

Test FAILed.

ethzasl-jenkins commented 5 years ago

Test PASSed.

ethzasl-jenkins commented 5 years ago

Test PASSed.

ethzasl-jenkins commented 5 years ago

Test PASSed.

ethzasl-jenkins commented 5 years ago

Test PASSed.

ethzasl-jenkins commented 5 years ago

Test PASSed.

hermannsblum commented 5 years ago

@michaelpantic I think this requires a bit more explanation as the ConfigProvider reminds me too much of this here ;)

What is the purpose? What problem does rosparam have that this tool solves? Why do we need it specifically for cad-percept?

@jstiefel How did you do the profiling of your code, and how do you rate the usability of the profiling here?

michaelpantic commented 5 years ago

@hermannsblum haha that link :-D

There's 2 problems I have with rosparam:

Soo this solution would just break the dependency on rosparam, but in most cases I would still use the RosConfigProvider that just reads stuff from rosparam (so nothing there changes). But with this solution I can read params wherever I want in my classes but its' decoupled from ROS and can be just set with a dummy configprovider for unit tests (if we ever get to write them ... dreams...)

Often people use gflags for this (e.g. maplab), but they are hard to set programmatically (ask marius)

TLDR: use this abstraction for libraries that shouldn't be tied too much to ROS, but in the actual nodes, use rosparam and pass a RosConfigProvider to the libraries.

michaelpantic commented 5 years ago

Also, this is not very cad-percept specific. But didn't find it everywhere else / don't want to have more dependencies.

But it does add another level of abstraction, that's true and can be confusing. On the other hand, the actual code that uses it looks same as if it would be using nodehandles.

michaelpantic commented 5 years ago

Btw about the profiling: I often use stuff like valgrind/cachgrind to profile relative performance (https://baptiste-wicht.com/posts/2011/09/profile-c-application-with-callgrind-kcachegrind.html), but the trouble with that is that the code has to be compiled in debug and runs about 1-2 orders of magnitude slower. But the profiling you get is very very detailed and helpful to figure out where time is spent (relatively seen).

So I added this profiler that is similar to what people use on microcontrollers/RTOS for simple profiling without influencing the runtime much.

michaelpantic commented 5 years ago

Alternatively, there are the perf tools (https://baptiste-wicht.com/posts/2011/07/profile-applications-linux-perf-tools.html) that don't have really overhead, but they only work on function level

hermannsblum commented 5 years ago

This all makes sense, lgtm :)

One suggestion for the future: It would be nice to add a function getParam that just fails if the parameter is not there. Often my code results in 'if hasParam then getParam else output ` etc, and it would be nice to push this pattern into the abstraction.

We can implement this in a later version tough.

Also for reference, maplab uses a rosparam to gflags converter to kind of solve a similar problem: https://gist.github.com/mfehr/18762943fb6af9710a5583f439dd9aff It comes with its own problems however, for now the config-class seems the easier option.

ethzasl-jenkins commented 5 years ago

Test PASSed.

michaelpantic commented 5 years ago

@hermannsblum Good point with the failing params! Added it quickly now. (at least now we have param (with default) / getParam (with bool as return) and hasParam as in Nodehandle). So should work for most cases.

I agree that it would be nice to have some handling of what you mention in the abstraction! But can add this part in a later version (would have to think of a good interface first).

hehe that solution was what I meant but didn't have the code somewhere ;-) It's quite a problem everywhere. Could consider finding the right level of abstraction in sw architecture an art ;-)

anyway, now I'm done with technicalities. back to actual meshrobotics.

ethzasl-jenkins commented 5 years ago

Test PASSed.