Code Quality about raspicam_node HOT 11 CLOSED

Thanks for opening this issue @SquirrelCZE, don't worry you are not being rude at all, code quality is always important.

Improving the overall code quality of raspicam_node is something that we have wanted to focus some time and energy on, but unfortunately hasn't become a priority.

A lot of the code hasn't been touched in a long time (since 2013), and on the verge of "legacy" with its unclear ownership semantics, and overall very C-like C++.

Since we are not really targeting anything before ROS kinetic, we should probably use all the new tools that C++11 provides us to write better code.

Anyway, if you want to contribute towards improving raspicam_node, I would be happy to assist you.

Is there anything specific that you would like to discuss about the code?

from raspicam_node.

Well, as long as we agree about what should be improved, I think this can work. And with your answer I think I can add this project on my TODO list.

I think it would be nice to pick/agree on things that should be improved and pointed out to anybody in way: "If you seen this in the code, you can refactor it".

Apart from the things pointed out in my previous post, ( pointers, global variables, bad datatypes ). I was thinking if you would mind usage of clang format tool ? (Just personal favourite) https://github.com/davetcoleman/roscpp_code_format

I think that for more complex changes (given that you seem to be willing to do them), I need to review the code properly and prepare some parts that can use a refactor.

As I said, I can't really do this alone and have to manage my time properly, so I would like to:

• write down complex changes and discuss them
• write down 'signs of bad code' that should be avoided and put it somewhere 'visible', so other people can either refactor some stuff or it can server as guidelines for patches
• work on these things as I have time for it, which I hope I will have enough

State, when it's actually visible: "we know there are some things that are not good, we accept that, please help us refactor it" could attract more people for help with this.

from raspicam_node.

from raspicam_node.

👍 on using clang-format. Some of our other code uses the ros2 style guide here: https://github.com/ros2/ros2/wiki/Developer-Guide#c-1

A clang-format file can be found here:
https://github.com/UbiquityRobotics/ubiquity_motor/blob/indigo-devel/.clang-format.

I think we can detail the things that need to be changed in a CONTRIBUTING.md file in the top of the repo, and include some information about why each change helps.

The "we know there are some things that are not good, we accept that, please help us refactor it" statement can go in the README, and point to the CONTRIBUTING.md file for more details.

Does this sound reasonable to you?

from raspicam_node.

from raspicam_node.

@jrlandau, while I share the general sentiment, I think it is important to work on improving this code for a few reasons.

1. In its current state, adding features or investigating issues is difficult, so we are spending way more time trying to understand the code when debugging than we should be.
2. raspicam_node is one of our most popular open-source repositories, so we should strive to keep it in a well-maintained condition.
3. Cleaning up the old cruft in raspicam_node may provide an opportunity to improve performance, or to reduce bugs in edge-cases.

from raspicam_node.

from raspicam_node.

@davecrawley We do not have any test coverage right now. The testing will have to be manual right now.

Adding automated testing should be a goal, but I am not sure how much is actually testable in the current state.

@SquirrelCZE Can we work together on compiling a list of improvements that can be made? I think a reasonable place to start would be to refactor the main stuff in raspicam_node.cpp into a class to get rid of the unnecessary global variables. What do you think?

from raspicam_node.

Sorry folks, I was busy:

@rohbotics I think I can help with that.

More than the global variables , I wam wondering about the fact that there are a lot of pretty long functions with pointer mechanics.

A. Could we ellaborate on where it would makes sense to split them? I would like to make this semantically correct, and I am not sure I can do that

• encoder_buffer_callback | that seems plittable
• create_camera_component | should definetly be multiple of small functions
• create_encoder_component | same here

B. functions like get_status in raspicam_node can be simplified.

• Loading parameters 'ifs' can be rewritten as one smart loading function executed multiple times

C. memory handling

• PORT_USERDATA * callback_data_enc = (PORT_USERDATA *) malloc (sizeof(PORT_USERDATA));

I think, emphasis should be done on these things at first, ie. working on stuff that increases readibility and getting rid of C style memory handling, as I think is the most 'dangerous' thing.

With simple code to read and understand, without dangerous problems like "what if we forget to free that memory", than I think we should move to getting rid of globals, making it more OOP or ... whatever.

As stated, without tests this is dangerous, and just splitting actual code into shorter, simpler functions, getting rid of manually managed memory and forms of code duplication, can be pretty interesting in itself.

Also, given that you state you aim for Kinetic, I think we can assume full C++11 usage? (which should be stated, as C++11 provides stuff like unique_ptr/shared_ptr which is nice to have)

also, I think that CONTRIBUTING.md is great idea.

from raspicam_node.

A.
encoder_buffer_callback could be split into the part that manages the mmal camera handling stuff, and the part that actually fills the CompressedImage. (Split out lines ~308-330).

create_camera_component I am not sure about. I don't think any of the still_camera_port stuff is being used at the moment, so maybe just get rid of it? I will need to look into this further, as well as do some testing.

create_encoder_component I am not sure where a good semantic point of separation is. It is setting up the settings and the memory buffers for the JPEG hardware encoder. A lot of the code is actually error handling.

B.
Can you elaborate on this please, it sounds interesting.

C.
This is probably the hardest yet most important one, but we need to be careful with our C++ memory management features when we are interfacing with the C mmal library.

Everything else sounds good, I will try to start working on a CONTRIBUTING.md document outlining the stuff that needs improvement.

from raspicam_node.

Here I am again.

https://github.com/SquirrelCZE/raspicam_node/commit/cc1e5ae6c73a7c6a3ad62bc85cd497d3064b7d4b

I finished the changes for loading parameters in my fork, I was only able to compile it, not test properly, however I think the point is pretty clear.

During the week I should be able to get hands on my rpi and test it in case you won't do it by that time, than I think I can slowly move to other things.

from raspicam_node.