Feature/dynamic reconfigure #12

4c3y · 2022-08-03T14:20:18Z

Added support for changing mode and clock frequency during runtime with dynamic reconfigure

michaelpantic

LGTM, see comments!

michaelpantic · 2022-08-15T11:42:10Z

time_stamper_ros/CMakeLists.txt

@@ -4,7 +4,7 @@ project(time_stamper_ros)
 add_compile_options(-std=c++17 -Wdeprecated-declarations)

 find_package(catkin_simple REQUIRED)
-catkin_simple()
+catkin_simple(ALL_DEPS_REQUIRED)




unecessary vspace

michaelpantic · 2022-08-15T11:45:12Z

time_stamper_ros/cfg/Led.cfg

+from dynamic_reconfigure.parameter_generator_catkin import *
+
+gen = ParameterGenerator()
+gen.add("frequency",    int_t,    0, "Clock frequency", 50,  1, 10526315)


hmm I wonder if it makes sense to change frequency completely freely as an integer, as usually not all frequencies can be achieved by the clock. Maybe a divisor makes more sense? Depends on the use case, though.
Make sure the default, min and max are correct.

Hm, this is difficult problem indeed. A divider may also be hard to work with for some scrip not knowing very well about the hardware. One possible way to deal with this is on the node side to change the requested value to a closest possible frequency (see my comment below for how to do that). For interactive configuration this should work well. The user would effectively only be able to adjust the value to something possible. And scripts may also be happy with that. If they really need to know precisely, they could read the effective value back and work with that.

michaelpantic · 2022-08-15T11:45:45Z

time_stamper_ros/cfg/Led.cfg

+            gen.const("Exposure", int_t, 1, "Exposure Mode")],
+            "Set Led Mode")
+
+gen.add("mode", int_t, 0, "Led Mode", 1, 0, 3, edit_method=size_enum)


3 as max value probably makes no sense given that there are only 2 values possible

michaelpantic · 2022-08-15T11:47:23Z

time_stamper_ros/include/Node.h

 enum LedMode {
-  FPS,
-  EXPOSURE
+  FPS = 0,
+  EXPOSURE = 1
 };


I'd prefer class enums, see https://www.geeksforgeeks.org/enum-classes-in-c-and-their-advantage-over-enum-datatype/

michaelpantic · 2022-08-15T11:49:43Z

time_stamper_ros/src/Node.cpp

+  mode_ = static_cast<LedMode>(config.mode); //Convert int to enum
+
+  pwm_subsystem_.setFrequency(config.frequency);
+  setGpioMode();


hmm I guess this sets the LedMode to the GPIO driver?
Nitpick, but for maintainability it would be better to have a comment and/or put this next to the mode_ assignment. So future Mariano or Hannes don't need to think too much about sideeffects when changing anything here.

Yeah, this isn't perfect this way. Without looking much into the code (lack of time :( ), to me the underlying problem seems to be again, the fact that this method doesn't take the mode (static_cast<LedMode>(config.mode)) as an argument. Wouldn't that make more sense? Then, here you wouldn't need to use the knowledge how the mode is stored (mode_). Maybe you wouldn't even need the mode_ variable if you could just directly pass it to the hardware. Generally, I'd recommend to try avoiding redundant state (and that includes hardware state in addition to memory state). Duplicating it may of course be justified by the hardware being to slow or not able to provide back the value in case it needed again somewhere later.

HannesSommer

I'm very sorry for the huge delay. Please, in future, ping until I react. It is never on purpose.

HannesSommer · 2022-10-07T08:00:08Z

time_stamper_ros/cfg/Led.cfg

+from dynamic_reconfigure.parameter_generator_catkin import *
+
+gen = ParameterGenerator()
+gen.add("frequency",    int_t,    0, "Clock frequency", 50,  1, 10526315)


Hm, this is difficult problem indeed. A divider may also be hard to work with for some scrip not knowing very well about the hardware. One possible way to deal with this is on the node side to change the requested value to a closest possible frequency (see my comment below for how to do that). For interactive configuration this should work well. The user would effectively only be able to adjust the value to something possible. And scripts may also be happy with that. If they really need to know precisely, they could read the effective value back and work with that.

HannesSommer · 2022-10-07T08:06:00Z

time_stamper_ros/src/Node.cpp

+  mode_ = static_cast<LedMode>(config.mode); //Convert int to enum
+
+  pwm_subsystem_.setFrequency(config.frequency);
+  setGpioMode();


Yeah, this isn't perfect this way. Without looking much into the code (lack of time :( ), to me the underlying problem seems to be again, the fact that this method doesn't take the mode (static_cast<LedMode>(config.mode)) as an argument. Wouldn't that make more sense? Then, here you wouldn't need to use the knowledge how the mode is stored (mode_). Maybe you wouldn't even need the mode_ variable if you could just directly pass it to the hardware. Generally, I'd recommend to try avoiding redundant state (and that includes hardware state in addition to memory state). Duplicating it may of course be justified by the hardware being to slow or not able to provide back the value in case it needed again somewhere later.

HannesSommer · 2022-10-07T08:25:29Z

time_stamper_ros/src/Node.cpp

+    return;
+  }
+  mode_ = static_cast<LedMode>(config.mode); //Convert int to enum
+


Around here you could mutate the config object and set the frequency to a closest valid value.

4c3y · 2022-10-27T15:25:58Z

Unfortunately I don't have time at the moment due to preparations for the Swiss Robotics Day. I'll have a look at it the week afterwards. AFAIK you can indeed freely set the clock frequency up to 10.5 MHz (At least sysfs is fine with it 😄).

4c3y · 2022-11-11T10:20:42Z

@HannesSommer As far as I can tell, it should be possible to freely set the clock frequency according to chapter 5.4 in the bcm2711 peripherals datasheet

4c3y added 2 commits August 3, 2022 15:47

Added dynamic reconfigure callback

ec5a05a

Bugfixed Node

0b5da6a

4c3y linked an issue Aug 3, 2022 that may be closed by this pull request

Allow changing the parameters (mode and clock frequency) of the timestamper device during runtime #9

Open

4c3y added the enhancement New feature or request label Aug 3, 2022

4c3y requested review from michaelpantic and HannesSommer August 3, 2022 14:21

michaelpantic approved these changes Aug 15, 2022

View reviewed changes

HannesSommer approved these changes Oct 7, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/dynamic reconfigure #12

Feature/dynamic reconfigure #12

4c3y commented Aug 3, 2022

michaelpantic left a comment

michaelpantic Aug 15, 2022

michaelpantic Aug 15, 2022

HannesSommer Oct 7, 2022

michaelpantic Aug 15, 2022

michaelpantic Aug 15, 2022

michaelpantic Aug 15, 2022

HannesSommer Oct 7, 2022

HannesSommer left a comment

HannesSommer Oct 7, 2022

HannesSommer Oct 7, 2022

HannesSommer Oct 7, 2022

4c3y commented Oct 27, 2022

4c3y commented Nov 11, 2022 •

edited

Loading

Feature/dynamic reconfigure #12

Are you sure you want to change the base?

Feature/dynamic reconfigure #12

Conversation

4c3y commented Aug 3, 2022

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

Choose a reason for hiding this 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 Oct 27, 2022

4c3y commented Nov 11, 2022 • edited Loading

4c3y commented Nov 11, 2022 •

edited

Loading