Add ROS output for the computer vision #10

4c3y · 2022-06-24T10:15:51Z

Changes

Added Ledstate.msg
Implemented led state publisher
Refactoring which closes Improve Configuration #7
Some bugfixes

… values to configuration.h

michaelpantic

See comments, otherwise looks good to me

michaelpantic · 2022-06-28T11:26:33Z

time_stamper_cv/include/image_processor.h

@@ -5,6 +5,8 @@
 #include "cv_bridge/cv_bridge.h"
 #include "led_state_parser.h"
 #include "keypoint_detector.h"
+#include <vector>


Check include orders as defined per google stylesheet: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

michaelpantic · 2022-06-28T11:28:16Z

time_stamper_cv/msg/Ledstate.msg

+bool is_valid
+int32 counter
+int32[] binary_state
+uint8[] intensity


as intensity is an averaged value of many uint8's (has more resolution than just a single uint8), you could also use a float/double representation here.

time_stamper_cv/include/configuration.h

michaelpantic · 2022-06-28T11:30:56Z

time_stamper_cv/src/image_processor.cpp

+
+  for (int i = 0; i < led_parser_->getLedRow(ImageProcessorConfig::TOP_ROW).size(); i++) {
+    double brightness = led_parser_->getLedBrightness(ImageProcessorConfig::TOP_ROW, i);
+    msg.intensity.push_back((uint8_t) std::lround(brightness));


see comment in message defnition

michaelpantic · 2022-06-28T11:32:07Z

time_stamper_cv/src/led_detection_node.cpp

+  img_pub_ = nh_.advertise<sensor_msgs::Image>("timestamper/image", 1);
+  led_state_pub_ = nh_.advertise<time_stamper_cv::Ledstate>("timestamper/led_state", 1);


instead of namespacing the topic name, its probably better to just name them image and led_state and let the namespacing be done via roslaunch

"Image" is ambiguous with topics from image_undistort, so I leave it like this

HannesSommer · 2022-06-28T12:06:57Z

time_stamper_cv/msg/Ledstate.msg

+Header header
+bool is_valid
+int32 counter
+int32[] binary_state


Why is the binary state an int32 (and not a bool)? What values can it have?

HannesSommer

Some ideas to improve code quality. On a functional level, I'm very happy with this. Thanks a lot!

HannesSommer · 2022-06-28T13:58:50Z

time_stamper_cv/src/image_processor.cpp

+    for (const auto &led: led_parser_->getLedRow(ImageProcessorConfig::BOTTOM_ROW)) {
      cv::Point2f led_pos = LedStateParser::normalize(led);
      cv::circle(visualization_mat, led_pos, (int) 10, cv::Scalar(255, 0, 0));
    }

-    for (const auto &led: led_parser_->getLedRow("TopRow")) {
+    for (const auto &led: led_parser_->getLedRow(ImageProcessorConfig::TOP_ROW)) {
      cv::Point2f led_pos = LedStateParser::normalize(led);
      cv::circle(visualization_mat, led_pos, (int) 10, cv::Scalar(255, 0, 0));
    }


I'd recommend avoiding such code duplication. What about having a for-each loop construct over the two rows?
E.g.

for(auto row: {ImageProcessorConfig::BOTTOM_ROW, ImageProcessorConfig::TOP_ROW}) { for(const auto &led: led_parser_->getLedRow(row)) { } }

Actually, even better would be to have some general support for iterating over all rows, because that is what you really want here, if I'm not mistaken. You don't really want this part of the code to depend on the fact that only there are two rows -- because it doesn't matter for it.

HannesSommer · 2022-06-28T14:02:33Z

time_stamper_cv/src/image_processor.cpp

+  for (int i = 0; i < led_parser_->getLedRow(ImageProcessorConfig::TOP_ROW).size(); i++) {
+    double brightness = led_parser_->getLedBrightness(ImageProcessorConfig::TOP_ROW, i);
+    msg.intensity.push_back((uint8_t) std::lround(brightness));
+    msg.binary_state.push_back(led_parser_->isLedOn(ImageProcessorConfig::TOP_ROW, i));
+  }
+
+  for (int i = 0; i < led_parser_->getLedRow(ImageProcessorConfig::BOTTOM_ROW).size(); i++) {
+    double brightness = led_parser_->getLedBrightness(ImageProcessorConfig::BOTTOM_ROW, i);
+    msg.binary_state.push_back(led_parser_->isLedOn(ImageProcessorConfig::BOTTOM_ROW, i));
+    msg.intensity.push_back((uint8_t) std::lround(brightness));
+  }


Same duplication here, I believe.

HannesSommer · 2022-06-28T14:20:14Z

time_stamper_cv/include/image_processor.h

+   * Message to be published
+   * @return Led state with set values
+   */
+  time_stamper_cv::Ledstate getLedStateMessage();


I smell a problematic design here since this function doesn't have any input. I tried to understand why and found that the processImage function of the LedStateParser changes its internal state. That isn't a good idea IMO. I don't have the space to explain it here but I'd highly recommend having a dedicated class representing the outcome of the an processImage and that processImage is a const member of the LedStateParser.

It is very uncommon for a parser to represent the parse outcome as internal state and there are many good reasons for that. I'm happy to explain one day in person. I think in very short, this is about separating data (led state, images) from actors/entities (e.g. a parser).

This function here, could then be a member of that outcome class iff its members would hold all relevant information. Otherwise consider passing such a parse outcome to it as an argument instead.

I mostly found a potential problem with the convex shape. I'm not sure what is_valid actually does, but is it actually interesting to receive led parser results when the convex shape is invalid? It sounds to me that parsing doesn't really make sense when the shape is invalid. Or does it?

The process function of the ImageProcessor could simply return both, the let-state and the Image message it currently returns (e.g. using a std::tuple or - probably better - a small struct holding both with proper names). More symmetric would probably be if the process simply returns both messages: the output image and the led state message.

I'd highly recommend having a dedicated class representing the outcome of the an processImage and that processImage is a const member of the LedStateParser

I`ll look into this next week. I've opened an new issue regarding this problem. (#11)

I mostly found a potential problem with the convex shape. I'm not sure what is_valid actually does, but is it actually interesting to receive led parser results when the convex shape is invalid? It sounds to me that parsing doesn't really make sense when the shape is invalid. Or does it?

This is a design choice, because there are some nodes, which require a message at a given rate regardless of being valid or not. is_valid is a flag to indicate validity.

HannesSommer · 2022-06-28T14:21:05Z

time_stamper_cv/include/image_processor.h

+   * Message to be published
+   * @return Led state with set values
+   */
+  time_stamper_cv::Ledstate getLedStateMessage();


I'd reserve get for getters. This function isn't a getter at all because it doesn't just get something that is already there. What about build ?

HannesSommer · 2022-06-28T14:38:18Z

time_stamper_cv/src/led_detection_node.cpp

+  time_stamper_cv::Ledstate  led_msg = image_processor_->getLedStateMessage();
+  led_msg.header = image.header;


I find it problematic that getLedStateMessage kind of claims to produce a complete message but the caller still has to patch in the head. What about going the other way around: the caller of getLedStateMessage creates the message and hands passes it on in a mutable way to it (e.g non-const ref). That would of course require a renaming of the function, to e.g. fillInLedState. I really don't like "output" parameters but returning something incomplete I find even worse :D. In an ideal world the message would be comprised of bigger junks, one of which this message would return. But that isn't really easy to achieve here, I believe.

That suggested pattern of a fillIn.. function would also solve the problem of the is_valid: it could be set by the caller (i.e. here or in the process function in case you move this into it).

4c3y · 2022-07-05T15:01:14Z

Thanks for the great feedback!

I addressed all requested changes (except the design of the LedStateParser) - please take a second look

michaelpantic

Looks good to me! - I think this can be merged, and then you can concentrate on the refactor.

About refactoring: Its a good time pre-refactoring to think about a comprehensive set of unit tests, so that during refactoring you can always verify that functionality stays the same.

(I'll also add a comment in the other PR)

HannesSommer

New changes look like a good improvement. Thanks for applying them. Some feedback that may help. More for the learning than for the code itself... Sorry, I can't help it :).

HannesSommer · 2022-07-12T15:43:20Z

time_stamper_cv/include/configuration.h

+    std::vector<std::string> res{};
+
+    std::map<std::string, LedRowConfig>::const_iterator it;
+    for (it = led_row_config.begin() ; it != led_row_config.end(); it++) {
+      res.push_back(it->first);
+    }
+    return res;


This you can greatly simplify, depending on your version of c++. I'd assume at least 11. Otherwise something is quite off here.

Suggested change

std::vector<std::string> res{};

std::map<std::string, LedRowConfig>::const_iterator it;

for (it = led_row_config.begin() ; it != led_row_config.end(); it++) {

res.push_back(it->first);

}

return res;

std::vector<std::string> res{};

for (const auto &item : led_row) {

res.push_back(item.first);

}

return ret;

With c++17 you can destructure the key-value pair (item) to make it nicer, for(const auto& [key, _] : led_rows) ....

With c++20 you could initialize from a view instead:

Suggested change

std::vector<std::string> res{};

std::map<std::string, LedRowConfig>::const_iterator it;

for (it = led_row_config.begin() ; it != led_row_config.end(); it++) {

res.push_back(it->first);

}

return res;

const auto kv = std::views::keys(led_rows);

return { kv.begin(), kv.end() };

c++23 will probably further simplify :) (.to). At least they are getting there :D.

Generally this pattern has a quite sub-optimal performance because it creates the same vector on each call. Not a sign. problem here, but you should know.
To address that I'd use a static variable and IIFE to initialize it. Here a c++20 compatible example but you can do even with c++11.

Suggested change

std::vector<std::string> res{};

std::map<std::string, LedRowConfig>::const_iterator it;

for (it = led_row_config.begin() ; it != led_row_config.end(); it++) {

res.push_back(it->first);

}

return res;

const static auto row_names = []() {

const auto kv = std::views::keys(led_rows);

return std::vector<std::string>{ kv.begin(), kv.end() };

}();

return row_names;

(IMPORTANT: this makes only sense if you change the return type to a const ref at the same time)

Another performance sub-optimality is the needless copy of a std::string. A vector of string_view s could make that better since c++17.
But no need to do that -- optimization is important to have a good basic feeling for but should usually be done with care -- if it make the code more complicated because that is usually the more problematic cost foremost because it leads to bugs!

HannesSommer · 2022-07-12T15:47:42Z

time_stamper_cv/src/image_processor.cpp

+  std::vector<std::string> rows = cfg_.rows();
+  std::for_each(rows.begin(), rows.end(), [this, &msg] (const std::string& name) {


You can still use a for-: loop saving you the rows variable:

for(const auto& name: cfg_.rows()) { ... }

HannesSommer · 2022-07-12T15:50:01Z

time_stamper_cv/src/image_processor.cpp

+
+  std::vector<std::string> rows = cfg_.rows();
+  std::for_each(rows.begin(), rows.end(), [this, &msg] (const std::string& name) {
+    for (int i = 0; i < led_parser_->getLedRow(name).size(); i++) {


This lookup led_parser_->getLedRow(name) indicates to me that actually, you should directly iterate over led_rows map's items (ideally with c++17 structured binding) to avoid the ugly .first and .second..

… ImageProcessor::fillInLedStateMessage

4c3y · 2022-08-03T08:08:58Z

I didn't know structured bindings existed, so thanks for pointing that out! I added the suggested changes above.

Added ledstate ros message and implemented publisher, moved hardcoded…

07a8efd

… values to configuration.h

4c3y added the enhancement New feature or request label Jun 24, 2022

4c3y requested a review from michaelpantic June 24, 2022 10:15

4c3y linked an issue Jun 24, 2022 that may be closed by this pull request

Add ROS output for the computer vision #8

Closed

4c3y requested a review from HannesSommer June 27, 2022 07:31

michaelpantic approved these changes Jun 28, 2022

View reviewed changes

HannesSommer reviewed Jun 28, 2022

View reviewed changes

HannesSommer approved these changes Jun 28, 2022

View reviewed changes

4c3y added 5 commits July 5, 2022 12:44

Minor bugfixes, Changed binary_state from int32 to bool array

872aa4e

Fixed include order in all files

46a01bf

Changed intensity value from uint8 to double

439483e

Code deduplication + Refactoring

0da2cac

Refactored ImageProcessor::fillInLedStateMessage

751bb0b

4c3y mentioned this pull request Jul 5, 2022

Refactor LedStateParser #11

Open

4c3y requested review from HannesSommer and michaelpantic July 5, 2022 15:01

michaelpantic approved these changes Jul 12, 2022

View reviewed changes

HannesSommer reviewed Jul 12, 2022

View reviewed changes

Changed C++ standard to C++17, Refactored configuration.h, Simplified…

f5a972f

… ImageProcessor::fillInLedStateMessage

4c3y merged commit ffd02d1 into main Aug 3, 2022

4c3y deleted the feature/cv_output branch August 3, 2022 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ROS output for the computer vision #10

Add ROS output for the computer vision #10

4c3y commented Jun 24, 2022 •

edited

Loading

michaelpantic left a comment

michaelpantic Jun 28, 2022

michaelpantic Jun 28, 2022

michaelpantic Jun 28, 2022

michaelpantic Jun 28, 2022

4c3y Jul 5, 2022

HannesSommer Jun 28, 2022 •

edited

Loading

HannesSommer left a comment

HannesSommer Jun 28, 2022

HannesSommer Jun 28, 2022

HannesSommer Jun 28, 2022

4c3y Jul 5, 2022

HannesSommer Jun 28, 2022

HannesSommer Jun 28, 2022

4c3y commented Jul 5, 2022

michaelpantic left a comment

HannesSommer left a comment

HannesSommer Jul 12, 2022

HannesSommer Jul 12, 2022

HannesSommer Jul 12, 2022

4c3y commented Aug 3, 2022

		img_pub_ = nh_.advertise<sensor_msgs::Image>("timestamper/image", 1);
		led_state_pub_ = nh_.advertise<time_stamper_cv::Ledstate>("timestamper/led_state", 1);

		time_stamper_cv::Ledstate led_msg = image_processor_->getLedStateMessage();
		led_msg.header = image.header;

		std::vector<std::string> rows = cfg_.rows();
		std::for_each(rows.begin(), rows.end(), [this, &msg] (const std::string& name) {

Add ROS output for the computer vision #10

Add ROS output for the computer vision #10

Conversation

4c3y commented Jun 24, 2022 • edited Loading

Changes

michaelpantic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HannesSommer Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

HannesSommer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4c3y commented Jul 5, 2022

michaelpantic left a comment

Choose a reason for hiding this comment

HannesSommer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4c3y commented Aug 3, 2022

4c3y commented Jun 24, 2022 •

edited

Loading

HannesSommer Jun 28, 2022 •

edited

Loading