Niuslar / Multichannel-Temperature-Control

5 stars 3 forks source link

Humidity controller #120

Open Vavat opened 2 years ago

Vavat commented 2 years ago

Need a controller similar to CTemperatureController to interface with BME280 and humidifier to control humidity in the growth chamber.

Vavat commented 2 years ago

@samkno1, this is where we discuss implementation and how we track the progress.

Vavat commented 2 years ago

@Niuslar, this is a demo for Sam on how to raise a discussion point. No need to reply, this is training.

Vavat commented 2 years ago

@Niuslar, @Vavat, I don't understand this particular point.

class hardwaremapper is confusing...
Vavat commented 2 years ago

Oh. that's legacy blah-blah...

samkno1 commented 2 years ago

@Niuslar @Vavat What do you think of using the temperature controller class as the base class instead of the controller class as the base class? Humidity is controlled indirectly via the evaporator. My main focus would then be reimplementing the run method and reading the humidity value.

Vavat commented 2 years ago

@Niuslar @Vavat What do you think of using the temperature controller class as the base class instead of the controller class as the base class? Humidity is controlled indirectly via the evaporator. My main focus would then be reimplementing the run method and reading the humidity value.

@samkno1, can you articulate what the advantages be? I can think of disadvantages, but cannot think of advantages of the top of my head. Here are what I think disadvantages are:

There is a distinct possibility that you are seeing some advantage that I missed. For example, there is shared functionality that you would have to reimplement in Humidity controller. If that is the case, we should discuss architectural structure. It might be that what we should do is create another intermediate class, say, CEnvironmentalController, which will implement all shared functionality of both Temperature and Humidity and then the child classes inherit from it. This is a clean and widely accepted design paradigm.

samkno1 commented 2 years ago

@Niuslar @Vavat What do you think of using the temperature controller class as the base class instead of the controller class as the base class? Humidity is controlled indirectly via the evaporator. My main focus would then be reimplementing the run method and reading the humidity value.

@samkno1, can you articulate what the advantages be? I can think of disadvantages, but cannot think of advantages of the top of my head. Here are what I think disadvantages are:

  • The code structure becomes "weird". Inheritance implies that Humidity controller somehow expands the functionality of Temperature controller, but in the physical world it does not, so the mimicry of class representing true nature of the physical object breaks.
  • It's less logical architecturally and more likely to cause confusion in the future. As I mentioned before the readability and mainteinability of the code is of paramount importance. As codebase grows larger the complexity has to be countered by clarity. It is more often than not beneficial to even sacrifice performance in favour of clarity.

There is a distinct possibility that you are seeing some advantage that I missed. For example, there is shared functionality that you would have to reimplement in Humidity controller. If that is the case, we should discuss architectural structure. It might be that what we should do is create another intermediate class, say, CEnvironmentalController, which will implement all shared functionality of both Temperature and Humidity and then the child classes inherit from it. This is a clean and widely accepted design paradigm.

I didn't think of code readability and architectural structure when using the CTemperatureController as the base class. A lot of the methods would apply to the humidity controller such as set temperature, get temperature, override heater...and I also just realized the methods are private not protected.

Vavat commented 2 years ago

@samkno1, you make a compeling point. There is a fair amount of overlap between temperature and humidity control. Here is what I suggest:

samkno1 commented 2 years ago

Sounds good, I'll continue using CController as the base class

samkno1 commented 2 years ago

@Niuslar @Vavat To interact with the humidity sensor, I need to include the CBME280 class in CRealHardware and IHardwareMap, right?

Vavat commented 2 years ago

@Niuslar @Vavat To interact with the humidity sensor, I need to include the CBME280 class in CRealHardware and IHardwareMap, right?

Ah, this is a really good question. So... architecturally we want as much abstraction of classes from real hardware as possible, so answer to your question is resounding yes. HOWEVER, you are deliberately swimming into the deeper end. In order to successfully incorporate CBME280 into hardware mapper class you are going to have to substantially modify 3 classes.

I would say there is good month of work for an experienced engineer. I am not saying you should not do that, but if you decide to go that route you should do it knowing how big a piece you are biting off.

Alternatively, you can implement humidity class with CBME280 being instantiated within it. Once you have that written and working correctly, we will merge your work into a develop and start hardware testing. In the meantime, you can start learning how to do abstraction of controller from hardware as outlined above. This also unblocks other workpackages. We are getting into a bit of higher level strategic work planning here, but we might as well. Subdividing work into small pieces not only allows you to more accurately gauge how much time each piece of work is going to take, but also more stuff gets unblocked and tested sooner. This reduces risk faster, because if there is a problem lurking in the unknown we'd rather know it sooner. Hope this makes sense.

samkno1 commented 2 years ago

Alright, I'll go with the latter option

samkno1 commented 2 years ago

@Niuslar I noticed there is a heater per channel and temperature is controlled at the different channels. For humidity control, will it be a similar setup (a humidity sensor per channel)? or will there be some central place where a humidity sensor will be placed?

Niuslar commented 2 years ago

@Niuslar I noticed there is a heater per channel and temperature is controlled at the different channels. For humidity control, will it be a similar setup (a humidity sensor per channel)? or will there be some central place where a humidity sensor will be placed?

@samkno1I think the humidity sensors will be placed separate from the temperature channels, but I'm not sure if we'll use one or several humidity sensors. I think it would be a good idea to make the code flexible enough to decide this later.

Niuslar commented 2 years ago

@samkno1 We're going to use only one humidity sensor.

Vavat commented 2 years ago

@samkno1, the uniformity of the thermal field is disrupted by leakage of heat through the walls of the incubator. Therefore, we have to compensate for these leaks by placing multiple sensors and multiple heaters. Humidity is not disrupted in the same way since water vapour is not going to leak out. Thus if thermal field is kept uniform, humidity is going to be uniform. Hope that makes sense.

samkno1 commented 2 years ago

@samkno1, the uniformity of the thermal field is disrupted by leakage of heat through the walls of the incubator. Therefore, we have to compensate for these leaks by placing multiple sensors and multiple heaters. Humidity is not disrupted in the same way since water vapour is not going to leak out. Thus if thermal field is kept uniform, humidity is going to be uniform. Hope that makes sense.

Yeah, I understand!

samkno1 commented 2 years ago

I'm having trouble trying to setup .clang-format. Can i get help on this?

MAhmedDev commented 2 years ago

I'm having trouble trying to setup .clang-format. Can i get help on this?

@samkno1 What have you tried ?

You need to install the cppcheck plugin image

Then provide the path to .clang-format image

samkno1 commented 2 years ago

I'm having trouble trying to setup .clang-format. Can i get help on this?

@samkno1 What have you tried ?

You need to install the cppcheck plugin image

Then provide the path to .clang-format image

So I do have all that set up. However, there is no automatic formatting done after I save my code. I've tried versions 13.0.0 and 9.0.0.

These are the steps I followed to set up clang:

  1. Cloned the LLVM 13.0.0 or 9.0.0 repository
  2. Installed CppStyle in STM32Cube IDE
  3. Set the path for .clang-format to the repository cloned
  4. Selected run on file save

Did I miss a step?

MAhmedDev commented 2 years ago

Hi @samkno1 You need to install LLVM https://releases.llvm.org/download.html Then link clang-format.exe I made a mistake where I linked .clang-format on the first screenshot, instead of clang-format.exe image

samkno1 commented 2 years ago

Thanks, Frozen02! It's working now