Feature/srm mains power control #123

Vavat · 2022-10-13T12:53:15Z

@samkno1, @Niuslar, please review the PR at your leisure.
There are several things for you to consider when reviewing.

This PR does not have an open ticket, therefore, ideally you want to insist on thorough explanation of what is happening, so you can match whether authors intentions were implemented correctly. Author should not force you to guess what PR is trying to achieve.
This PR is a collection of a small feature + a lot of small refactoring jobs. This is normally frowned upon. One could argue that a better approach would have been to postpone the feature implementation until refactoring of hardware maps and Controller base class was done, then implement a feature on top. This would have resulted in 3 PRs instead of 1 large. The opposing view would be that refactoring without context of a new feature would have been confusing. See if you can formulate an opinion whether you see merits in both approaches. We can discuss it after.

Purpose of PR:

Implement mains control PWM hardware map methods.
Implement low level access API to control mains PWM.
As a result of the above certain refactoring had to be done to CController, IHardwareMap, and two implementations of the hardware map virtual class.

…hod.

Niuslar

There's only one thing I think we might need to check. The rest looks good, thanks.

Niuslar · 2022-10-13T17:55:16Z

Controllers/CDebugController.cpp

                                   uint32_t run_period_ms)
    : CController(name, run_period_ms),
-      m_breather_light(BREATHING_GPIO_Port, BREATHING_Pin)
+      mp_hw(p_hardware_map)


I think we should check that this is not anullptr somewhere.

Niuslar · 2022-10-13T18:18:39Z

Controllers/CDebugController.cpp

-    m_breather_light.toggle();
+    static float breathing_light_intensity = 0;
+    mp_hw->setBreathingLight(breathing_light_intensity);
+    breathing_light_intensity = breathing_light_intensity ? 0 : 100;


If I understood correctly, this function sets the duty cycle to 0 or 100 to turn the LED on/off every time run() is called?

Yes. This is really just a plug for when we get around to beautifying it. Ideally, we would want CDebugController to be able to switch this light from slow flash for "Everything is OK" to other types of flashing for when something is the matter and might require attention. But this is frills and cannot distract us right now.

Niuslar · 2022-10-13T18:22:21Z

Controllers/CDebugController.cpp

+                           etl::format_spec(),
+                           true);
+            CController::s_scratch_pad += ": ";
+            etl::to_string(mains_power,


It would be great to have an overloaded "addToString()" function that adds floats, ints, and string to the buffer before calling send(). I might have a look this weekend.

That'd be awesome. Like c# and java do.

What might be a good thing to do is create a child class based on etl::string and expand it with ability to absorb numbers.
Another option is to expand send() method within CComsController to be able to accept parameters like sprintf() does.

Niuslar · 2022-10-13T18:27:53Z

Controllers/CDebugController.cpp

+        mains_channel = (uint8_t)(*p_command)[0];
+        if (mains_channel != (*p_command)[0])
+        {
+            return (ICommand::ERROR_TYPE_MISMATCH);


Will this ever be true? How can mains_channel != (*p_command)[0] if we are assigning it that value the line before?

This is a bit of an awkward way of seeing if an argument is a float. Horrible solution, but I couldn't come up with a better one. Open to suggestions.

One option I can think of is to simply cast whatever is in the first argument into unsigned int and then range check. If it was sent as float, then we ignore the decimal part and use it as if it was an integer. This will not cause failures, but it'd be possible to request channel 2.5 to switch to 100% power, which will switch channel 2 or will bounce back with error depending on how casting works.

Niuslar · 2022-10-13T18:53:52Z

Hardware/CRealHardwareMap.cpp

+        power = 100;
+    }
+    uint16_t ccr = p_timer->Instance->ARR * mainsPowerCorrection(power) / 100;
+    p_timer->Instance->CCR1 = ccr;


This doesn't change anything, but the HAL has a __HAL_TIM_SET_COMPARE(__HANDLE__, __CHANNEL__, __COMPARE__) "function" that sets the CCR register.

I dislike macros. Any compelling reason I should use your suggestion?

Niuslar · 2022-10-13T20:03:12Z

Controllers/CDebugController.cpp

+        mains_power = (*p_command)[1];
+    }
+    // range check arguments.
+    if ((mains_channel > 2) || (mains_power < 0) || (mains_power > 100))


I think we need to fix this because if _command->getArgumentCount() is not 1 or 2, then mains_channel and mains_power will be uninitialised.

That's because I forgot final else{}. Fixed.

Vavat added 15 commits October 2, 2022 14:55

feature: configure mains PWM outputs and zero crossing interrupt.

ce8cf62

feature: change zero-crossing detection to both edges.

4cb3601

feature: add mains zero crossing IRQ handler.

f454477

feature: add mains power control commands to hardware map.

51d42c5

feature: implement mains power correction.

5fc68a6

feature: add commonly used responses to parent class.

6063ebe

refactor (CTemperatureController): use newly created parent class met…

61a59c4

…hod.

feature (CDebugController): add hardware map to debug controller

70ab2da

fix (CRealHardwareMap): missing qualifier for method.

f4ce23e

refactor (Hardware maps): to reduce confusion with method names.

a8a2817

fix: bug in method call

7b57b54

feature (CDebugController): add debug API command to control mains PWM.

4c265fb

feature: add getter method for mains PWM power.

31bb422

feature (CDebugController): add command to query mains power.

e1e2383

refactor (CDebugController): to use hardware map for breathing light

95c2189

Vavat requested review from Niuslar and samkno1 October 13, 2022 12:53

Niuslar reviewed Oct 13, 2022

View reviewed changes

Vavat added 2 commits October 14, 2022 14:13

fix (CDebugController): review issues.

5bf67a0

fix: typo.

12cd7e0

Vavat requested a review from Niuslar October 16, 2022 09:19

Feature/srm mains power control #123

Are you sure you want to change the base?

Feature/srm mains power control #123

Uh oh!

Conversation

Vavat commented Oct 13, 2022

Uh oh!

Niuslar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vavat Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vavat Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Vavat Oct 14, 2022 •

edited

Loading

Vavat Oct 14, 2022 •

edited

Loading