Samk humidity control by samkno1 · Pull Request #121 · Niuslar/Multichannel-Temperature-Control

samkno1 · 2022-08-28T21:00:48Z

Humidity controller created.

Assumptions:

Single humidity sensor.

feature: add sensor data access methods.

Vavat

Sam, this is very good. Nicely done. Let's have pair programming session sometime next week to go through key problems. Specifically, ::run() method. We can then discuss hardware testing. I am in the process of building actual chamber, so this is the right time I think.

Vavat · 2022-09-17T15:10:47Z

+#include "CPIDLoop.h"
+#include "IHardwareMap.h"
+
+#define CHANNEL_NUMBER 8


Why do we have multiple channels?

In the temperature controller code, it looks like a heater is assigned per channel. In my humidity controller code, I utilize all the heaters in changing the humidity.

Vavat · 2022-09-17T15:15:19Z

+private:
+    void sendStatus(IComChannel *p_comchannel);
+    ICommand::command_error_code_t setHumidity(ICommand *p_command);
+    ICommand::command_error_code_t overrideHeater(ICommand *p_command);


I see you made an assumption that we are going to use heater as a humidifier. For now this is acceptable, but since part of the goal here is to educate you I thought I'd be appropriate to demonstrate how this would be working in a more advanced object oriented design.
Since we right now don't know what humidifier we are going to use, a more flexible design decision would have been to have another class
CHumidifier with methods such as CHumidifier::startEvaporation() and CHumidifier::stopEvaporation() or something along those lines.
Having said that, your design is fairly easy to adapt to having external humidifier and we will perhaps do that once this bit is done.

Vavat · 2022-09-17T15:16:34Z

+
+    IHardwareMap *mp_hw;
+
+    CBME280 *hum_sensor;


This member variable should be called CBME280* mp_humidity_sensor
mp_ prefix means that it's a member pointer. Also, we try to avoid shortening names since it detracts from readability.

Vavat · 2022-09-17T15:16:52Z

+
+    CBME280 *hum_sensor;
+
+    float target_humidity;


m_target_humidity

Vavat · 2022-09-17T15:18:49Z

+        {
+            if (actual_humidity != target_humidity)
+            {
+                if (actual_humidity < target_humidity)


This entire if... else... construct is unnecessary. CPIDLoop class will take care of it automatically if you set it up correctly. Let's discuss this part of the code in pair programming session.

Vavat · 2022-09-17T15:20:57Z

+    for (int i = 0; i < CHANNEL_NUMBER; i++)
+    {
+        float power = 0;
+        actual_humidity = hum_sensor->getHumidity();


You don't need to call humidity sensor withing the loop. The value of actual humidity is not going to change, so you are just wasting cycles. This entire construct is a bit confusing to me, so perhaps we should have a pair programming session to discuss what you were trying to achieve and how to best approach it.

Yes, we can talk about it during the session.

Vavat · 2022-09-17T15:22:37Z

+        }
+        mp_hw->setHardPwmOutput(power, i);
+    }
+    while (actual_humidity != target_humidity)


::run() method must exist. This version of the method hogs the control until humidity is reached. The superloop design pattern assumes that each controller will execute ::run() method to do quick work and exit. Looping like you did will likely cause a lock up since nothing else is getting processed.

I see, didn't think of that

Vavat · 2022-09-17T15:27:25Z

+    bool b_command_recognised = false;
+    ICommand::command_error_code_t result = ICommand::COMMAND_OK;
+    /**
+     * Query about current state of all active channels.


One of the things you will need to learn is where to comment. One habit that is worth developing is avoiding comments. If you feel like a piece of code requires comment, then first thing you should consider is whether you should refactor it to make code very clear and not require comments.
This type of comment is superfluous. Your code is very clean and easy to read, so cluttering it with comments that do not really add new information is detrimental.
Generally, comments should be placed where complex piece of logic is happening, e.g. you're doing some numerical approximation using your own algorithm. Or you have implemented another algorithm in which case brief description is in order and then a reference to a book or a website where algorithm is described in details. Another possibility for comments are in places where you have optimised the if... else construct for execution speed and this reduced readability, but you really want speed improvement, so explain what you've done.
This will come with experience, so don't worry about it too much now, but try to follow the principle.

Vavat · 2022-09-17T15:31:51Z

+}
+
+/**
+ * @brief Set humidity value


Incomplete doxy header. Should have
@param p_command field.
We need to make sure you have right setup to do automatic doxygen headers generation.

Vavat · 2022-09-20T17:32:53Z

@samkno1, when you address the issues raised during the code review, you can mark them as resolved.

samkno1 · 2022-09-30T16:19:19Z

@Vavat I intend on implementing the humidifier class to control the humidifier device. Since this is the case, do I currently need to worry about including the IHardwareMap class in my code since all access to external hardware will go through either the CHumidifier class or the CBME280 class

Vavat · 2022-10-01T12:09:10Z

@samkno1, when you think you have addressed all review comments and pushed new code you need to request review again. This restarts review process with your new code. Three are circular arrows next to the reviewers name - mine in this case - that will notify me that new changes are ready for review.

Vavat · 2022-10-01T12:14:20Z

@samkno1, when you think you have addressed all review comments and pushed new code you need to request review again. This restarts review process with your new code. Three are circular arrows next to the reviewers name - mine in this case - that will notify me that new changes are ready for review.

@samkno1, I have clicked the request review, but then realised you may not have pushed everything, so let's follow proper process. When you think code is ready, let me know.

samkno1 · 2022-10-01T22:52:03Z

Hi Vavat, It isn’t ready yet maybe I might have pushed to the wrong branch. Last week was really busy for me cause of school and there was a hurricane. I’ll request a new review once it’s ready, hopefully this week. Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for Windows From: Salavat ***@***.***> Sent: October 1, 2022 9:14 AM To: ***@***.***> Cc: Samuel ***@***.***>; ***@***.***> Subject: Re: [Niuslar/Multichannel-Temperature-Control] Samk humidity control (PR #121) CAUTION: The Sender of this email is not from within Dalhousie. @samkno1<https://github.com/samkno1>, when you think you have addressed all review comments and pushed new code you need to request review again. This restarts review process with your new code. Three are circular arrows next to the reviewers name - mine in this case - that will notify me that new changes are ready for review. @samkno1<https://github.com/samkno1>, I have clicked the request review, but then realised you may not have pushed everything, so let's follow proper process. When you think code is ready, let me know. — Reply to this email directly, view it on GitHub<#121 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AQI7YBGSZTL2ATURR6CEP3TWBATKRANCNFSM573V2HAQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Vavat · 2022-10-02T11:42:23Z

@samkno1, in the meantime I am going to create a humidifier interface within the hardware maps. You can choose to merge them to your code when they are ready.

samkno1 · 2022-10-02T13:06:14Z

@samkno1, in the meantime I am going to create a humidifier interface within the hardware maps. You can choose to merge them to your code when they are ready.

Sounds good

Feature/srm humidifier hardware

samkno1 · 2022-10-16T12:33:55Z

@Vavat You mentioned the runtime for which CDispatcher will run the humidity controller. Is the run time something I need to define somewhere?

samkno1 · 2022-10-16T12:38:18Z

I have the CPIDLoop defining the power variable for the humidifier

@Vavat You mentioned the runtime for which CDispatcher will run the humidity controller. Is the run time something I need to define somewhere?

Never mind. I just realized I included it as an argument in the Humidity controller class constructor.

…classes

Vavat

Let's have a pair programming session as soon as you can.

Vavat · 2022-10-17T15:31:24Z

@@ -8,6 +8,7 @@
 #include "CHumidityController.h"
 #include <stdio.h>
 #include "CBME280.h"


The header files that are included in CHumidityController.h do not need to be included here. They will be visible. Only include here header file for own class and whatever headers you need to operate, e.g. stdio.h.

Noted. I was a bit confused about including header files in the .cpp and .h. Thanks

Vavat · 2022-10-17T15:32:08Z

 #include "CHumidityController.h"
 #include <stdio.h>
 #include "CBME280.h"
+#include "CHumidifier.h"


Is this used? This might have been something legacy or something I introduced. Check if it does anything and if not remove.

No, I included that while I was previously using the CHumidifier class to do the humidifier abstraction. I forgot to remove it since it's no longer being used.

Vavat · 2022-10-17T15:34:22Z


-    CBME280 hum_temp_reader(p_spi, p_slave_select_port, slave_select_pin);
-    hum_sensor = &hum_temp_reader;
+    CBME280 humidity_sensor(p_spi, p_slave_select_port, slave_select_pin);


What is the point of these operations? Line 36 already creates humidity_sensor, so reassigning its pointer to another variable is useless operation. Am I missing something?

I think this is related to your comment on the SPI port not being set up. When I directly use the CBME280 humidity_sensor object. I get the following error "error: 'humidity_sensor' was not declared in this scope". So I opted for dynamically accessing the class with the m_humidity sensor pointer to the CBME280 class.

Vavat · 2022-10-17T15:36:56Z

@@ -32,8 +33,8 @@ CHumidityController::CHumidityController(IHardwareMap *p_hardware,
    // TODO Auto-generated constructor stub


Just realised that this constructor is not going to work because when CHumidityController is created, the SPI port hardware will not be setup yet, so the constructor pointers will be all nullptr. This is actually quite important aspect of c++ for you to understand, so let's have a pair programming session. Drop me an invite.

Vavat · 2022-10-17T15:47:45Z

-    float actual_humidity;
-    for (int i = 0; i < CHANNEL_NUMBER; i++)
+    float power = 0;
+    if (m_power_override == DISABLE_OVERRIDE)


I would recommend that special numbers are not used. They can be confusing. I would recommend that you create a special member variable bool mb_humidifier_override;. Move power variable from ::run() method into becoming actual member variable m_power. Then logic becomes much cleaner.

if (!mb_humidifier_override) { ... here you do the control of power since power is not overriden. } mp_hw->setHumidifierPower(m_power);

m_power is set to override value when command to override is executed.
This is much cleaner to read and is much less likely to cause confusion with API.

when processing commands, if you get a command that overrides the power, then you simply toggle mb_power_override.

Vavat · 2022-10-17T16:07:30Z

+    }
+    float power = *p_command;
+
+    // Execute command


Unnecessary comment. And it's wrong, since you are sanitising before executing.

Vavat · 2022-10-17T16:08:36Z

+    {
+        return ICommand::ERROR_ARG_COUNT;
+    }
+    float power = *p_command;


This is a mistake. To access first argument of the command it should be:

float power = (*p_command)[0];

Vavat · 2022-10-17T16:09:47Z

+    {
+        power = MAX_POWER;
+    }
+    if ((power < MIN_POWER) && (power != DISABLE_OVERRIDE))


This is complex logic and would be much easier if you used boolean switch to control override.

Vavat · 2022-10-17T16:10:48Z

+    }
+    if (b_command_recognised)
+    {
+        switch (result)


There is now a method in CController that will do what this switch statement does, so you can take advantage of it.

Vavat · 2022-10-17T16:13:56Z

+
+void CHumidityController::sendStatus(IComChannel *p_comchannel)
+{
+    etl::string<MAX_STRING_SIZE> message;


We are currently discussing how to return responses to be understood by higher layers. This might change drastically in the future.

Vavat and others added 17 commits July 24, 2022 13:11

feature: add module for BME280 sensor.

674806b

feature: transfer Sam's code unmodified to a new module.

1e3a824

feature: add sensor initialisation method

852b544

docs: add doxygen headers to methods.

7c55e7f

feature: add sensor data access methods.

feature: add conversion of raw data stream into raw structured data.

4780510

feature: add basic state machine to run sensor.

59dfe1f

wip

65d9806

wip.

0f24701

feature: add ADC data receive functionality.

65218ba

feature: enable IRQ for SPI2 engine.

661c567

feature: add IRQ callback via static method.

e260093

wip.

b1a5ac6

feature: modify calibration from integer to float.

4254bc6

feature: implement raw data calibration for T, P, and H measurements.

d37fde7

central sensor version

53006bd

feature: humidity controller setup

7da5c58

feature: humidity controller

c28330b

samkno1 requested review from Niuslar and Vavat August 28, 2022 21:25

Merge branch 'develop' into samk_humidity_control

1d31b19

Vavat requested changes Sep 17, 2022

View reviewed changes

WIP: Humidity controller

b9f5c75

Vavat self-requested a review October 1, 2022 12:09

feature: add humidifier interface methods to the hardware maps.

7541937

feature: add real hardware map implementation of the humidifier.

fd3b55f

Merge pull request #122 from Niuslar/feature/srm_humidifier_hardware

609e31d

Feature/srm humidifier hardware

samkno1 added 2 commits October 16, 2022 09:45

Implemented with 1 channel and humidifier abstracted by Hardware Map …

eabb573

…classes

Fixed commenting

b829813

Vavat requested changes Oct 17, 2022

View reviewed changes

samkno1 marked this pull request as draft December 26, 2022 20:01

samkno1 added 2 commits December 29, 2022 18:52

fix: HumidityController

8063c7a

fix: HumidityController

b28c1b0

Vavat self-requested a review February 22, 2023 14:29

		@@ -32,8 +33,8 @@ CHumidityController::CHumidityController(IHardwareMap *p_hardware,
		// TODO Auto-generated constructor stub

Conversation

samkno1 commented Aug 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vavat 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!

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!

Choose a reason for hiding this comment

Uh oh!

Vavat commented Sep 20, 2022

Uh oh!

samkno1 commented Sep 30, 2022

Uh oh!

Vavat commented Oct 1, 2022

Uh oh!

Vavat commented Oct 1, 2022

Uh oh!

samkno1 commented Oct 1, 2022 via email

Uh oh!

Vavat commented Oct 2, 2022

Uh oh!

samkno1 commented Oct 2, 2022

Uh oh!

samkno1 commented Oct 16, 2022

Uh oh!

samkno1 commented Oct 16, 2022

Uh oh!

Vavat 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!

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!

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!

samkno1 commented Aug 28, 2022 •

edited

Loading