ROS 2 lights converter #223

KmakD · 2024-02-06T14:22:05Z

This is a basic converter and most likely its functionality may change and be extended. Right now it is hard to predict what will be required by controller and I want to avoid rewriting parts of code.
The converter has only one simple method, this may change when implementing controller that's why I've put it inside separate files.

pkowalsk1

I really like the structure of this package. I pointed out a few minor things that, in my opinion, would look better. What are we doing with the header files? "" or <>? xd

pkowalsk1 · 2024-02-09T11:17:02Z

panther_lights/include/panther_lights/led_panel.hpp

@@ -0,0 +1,42 @@
+// Copyright 2023 Husarion sp. z o.o.


Should we update it to 2024?

pkowalsk1 · 2024-02-09T11:22:11Z

panther_lights/include/panther_lights/led_segment.hpp

+{
+public:
+  /**
+   * @brief Virtual LED segment of the robot


I prefer to move this description to the class brief. In my opinion, it would be better to add short descriptions to all or most impoertant classes in this package as it becomes more complex.

pkowalsk1 · 2024-02-09T11:29:38Z

panther_lights/src/led_segment.cpp

+
+#include <panther_lights/animation/animation.hpp>
+
+#include <iostream>


Is it unused?

pkowalsk1 · 2024-02-09T11:34:10Z

panther_lights/include/panther_lights/led_panel.hpp

+
+  ~LEDPanel() = default;
+
+  void UpdateFrame(const std::size_t iterator_first, const std::vector<std::uint8_t> & values);


It's just a suggestion, but maybe we could add a short description and information about exceptions here?

pkowalsk1 · 2024-02-09T11:40:31Z

panther_lights/include/panther_lights/led_segment.hpp

+  std::size_t first_led_iterator_;
+  std::size_t last_led_iterator_;


Are you sure it is an iterator? Maybe something similar to first_led_position_?

Methods from algorithm library use "iterator" naming and I was using this logic

I'm fine with iterator 🤓

pkowalsk1 · 2024-02-09T12:19:35Z

panther_lights/test/test_led_segment.cpp

+  auto segment_desc =
+    YAML::Load("{led_range: 0-" + std::to_string(segment_led_num_ - 1) + ", channel: 1}");


Suggested change

auto segment_desc =

YAML::Load("{led_range: 0-" + std::to_string(segment_led_num_ - 1) + ", channel: 1}");

const auto segment_desc =

YAML::Load("{led_range: 0-" + std::to_string(segment_led_num_ - 1) + ", channel: 1}");

pkowalsk1 · 2024-02-09T12:21:59Z

panther_lights/test/test_led_segment.cpp

+  led_segment_ = std::make_shared<panther_lights::LEDSegment>(segment_desc, controller_freq);
+}
+
+// this method may be moved to utils


It's a good idea. Should we do it now?

Looked at it and it may be a bit tricky - test_utils have dependencie on rclcpp. There is no point in adding rclcpp dependencies in here. The best solution, I guess, is to devide test_utils into 2 files(for ros and non ros utils). This however makes need for changes in other repositories using current test_utils and I dont nkow if it is a good idea to make them in this PR

ok, so it's up to you

I'll make another PR with this

pkowalsk1 · 2024-02-09T12:23:39Z

panther_lights/test/test_led_segment.cpp

+
+TEST(TestLEDSegmentInitialization, ValidDescription)
+{
+  auto segment_desc = YAML::Load("{led_range: 0-10, channel: 1}");


Suggested change

auto segment_desc = YAML::Load("{led_range: 0-10, channel: 1}");

const auto segment_desc = YAML::Load("{led_range: 0-10, channel: 1}");

pkowalsk1 · 2024-02-09T12:27:04Z

panther_lights/test/test_led_segment.cpp

+
+TEST(TestLEDSegmentInitialization, DescriptionMissingRequiredKey)
+{
+  auto segment_desc = YAML::Load("");


Could we move it to function like CreateSegmentDescription() in test_segment_converter.cpp?

pkowalsk1 · 2024-02-09T12:28:11Z

panther_lights/test/test_segment_converter.cpp

+  auto anim_desc = YAML::Load(
+    "{type: panther_lights::ImageAnimation, "
+    "image: $(find panther_lights)/animations/triangle01_red.png, "
+    "duration: 2}");


Same with this. Could we move it to function?

…verter

panther_utils/include/panther_utils/test/test_utils.hpp

panther_utils/include/panther_utils/test/ros_test_utils.hpp

panther_utils/test/test_test_utils.cpp

panther_lights/include/panther_lights/animation/animation.hpp

panther_lights/src/controller_node.cpp

panther_lights/src/segment_converter.cpp

panther_lights/src/led_segment.cpp

panther_lights/include/panther_lights/led_panel.hpp

panther_lights/test/test_led_panel.cpp

…panther_ros into ros2-lights-converter

* ROS 2 lights animations (#221) * add animation and image_animation class * controller node and pluginlib * add tests and fix issues * add animation images * add alpha channel * add charging animation with tests * update dummy controller * fix missing includes * add missing dep * Update panther_lights/include/panther_lights/animation/animation.hpp Co-authored-by: Paweł Kowalski <82044322+pkowalsk1@users.noreply.github.com> * Update panther_lights/include/panther_lights/animation/animation.hpp Co-authored-by: Paweł Kowalski <82044322+pkowalsk1@users.noreply.github.com> * review changes * update tests --------- Co-authored-by: Paweł Kowalski <82044322+pkowalsk1@users.noreply.github.com> * ROS 2 lights converter (#223) * add led_segment * WIP led_panel and segment converter * simplify converter * update segment conversion * add test for led panel, segment, and converter * review fixes * update copyright year * update controller so it somehow works * Update tests * Apply review fixes * fix gpio tests * parse controller configuration * add default animation * add yaml_utils to panther_utils * add led animation and queue * Fix queuing * fix bug * priority and timeout queue validation * move queue to separate file * add briefs * param and brightness handle * user animations, bugs, briefs * use yaml utils * fix tests * update tests * add led_animation test * test fixxes * add led animations queue tests * clean up code | clean up code * Update documentation | add launching controller node * make it work * update scheduler * Update panther_lights/LIGHTS_API.md Co-authored-by: Paweł Irzyk <108666440+pawelirh@users.noreply.github.com> * review fixes * update pre-commit and fix typos * Update panther_bringup/README.md Co-authored-by: rafal-gorecki <126687345+rafal-gorecki@users.noreply.github.com> * Update panther_hardware_interfaces/README.md Co-authored-by: rafal-gorecki <126687345+rafal-gorecki@users.noreply.github.com> * Update panther_lights/README.md Co-authored-by: rafal-gorecki <126687345+rafal-gorecki@users.noreply.github.com> * Update panther_lights/test/test_controller_node.cpp Co-authored-by: rafal-gorecki <126687345+rafal-gorecki@users.noreply.github.com> * review fixes * Update README.md --------- Co-authored-by: Paweł Kowalski <82044322+pkowalsk1@users.noreply.github.com> Co-authored-by: Paweł Irzyk <108666440+pawelirh@users.noreply.github.com> Co-authored-by: rafal-gorecki <126687345+rafal-gorecki@users.noreply.github.com>

KmakD added 5 commits January 30, 2024 14:36

add led_segment

607365f

WIP led_panel and segment converter

954d162

simplify converter

e09f676

update segment conversion

1244743

add test for led panel, segment, and converter

20b1ce9

JanBrzyk requested review from pkowalsk1 and pawelirh February 8, 2024 10:52

pkowalsk1 suggested changes Feb 9, 2024

View reviewed changes

KmakD added 2 commits February 13, 2024 08:56

review fixes

9198870

update copyright year

de1d45e

KmakD requested a review from pkowalsk1 February 13, 2024 11:44

KmakD added 3 commits February 13, 2024 11:57

update controller so it somehow works

8cf3992

Merge remote-tracking branch 'origin/ros2-devel' into ros2-lights-con…

a36c30a

…verter

Update tests

2451532

pkowalsk1 approved these changes Feb 14, 2024

View reviewed changes

pawelirh suggested changes Feb 16, 2024

View reviewed changes

KmakD added 3 commits February 19, 2024 10:17

Apply review fixes

f129f0a

Merge branch 'ros2-lights-controller' of https://github.com/husarion/…

7a05ca9

…panther_ros into ros2-lights-converter

fix gpio tests

5f5f144

KmakD requested a review from pawelirh February 19, 2024 10:39

pawelirh approved these changes Feb 20, 2024

View reviewed changes

KmakD merged commit 3c2ad2d into ros2-lights-controller Feb 20, 2024

KmakD deleted the ros2-lights-converter branch February 20, 2024 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROS 2 lights converter #223

ROS 2 lights converter #223

KmakD commented Feb 6, 2024

pkowalsk1 left a comment

pkowalsk1 Feb 9, 2024

pkowalsk1 Feb 9, 2024

pkowalsk1 Feb 9, 2024

pkowalsk1 Feb 9, 2024

pkowalsk1 Feb 9, 2024

KmakD Feb 12, 2024

pawelirh Feb 16, 2024

pkowalsk1 Feb 9, 2024

pkowalsk1 Feb 9, 2024

KmakD Feb 13, 2024

pkowalsk1 Feb 13, 2024

KmakD Feb 13, 2024

pkowalsk1 Feb 9, 2024

pkowalsk1 Feb 9, 2024

pkowalsk1 Feb 9, 2024


		#include <panther_lights/animation/animation.hpp>

		#include <iostream>


		~LEDPanel() = default;

		void UpdateFrame(const std::size_t iterator_first, const std::vector<std::uint8_t> & values);

		std::size_t first_led_iterator_;
		std::size_t last_led_iterator_;

		auto segment_desc =
		YAML::Load("{led_range: 0-" + std::to_string(segment_led_num_ - 1) + ", channel: 1}");

	auto segment_desc = YAML::Load("{led_range: 0-10, channel: 1}");
	const auto segment_desc = YAML::Load("{led_range: 0-10, channel: 1}");

ROS 2 lights converter #223

ROS 2 lights converter #223

Conversation

KmakD commented Feb 6, 2024

pkowalsk1 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment