Gw dev

[psmgeelen]

Lets Review!

[psmgeelen]

@omonrise , can you also have a look, my C++ skills are not great ;)

[psmgeelen]

@GreenWizard2015 , so my understanding is that you modularized everything and created structs to contain the information in a more transparent and better defined way. I can see that we now have 2 modules:

• WaterPumpController
• RemoteControl

I which you contain the logic of pouring the tea and setting up the Wifi connection and webserver respectively. @omonrise is going to give this a thorough look tomorrow as he is better with C++ then I am. I will test this PR on the device itself, to see whether we have any integration issues. @GreenWizard2015 I found an interesting page regarding unit-tests here: https://docs.platformio.org/en/latest/advanced/unit-testing/index.html Maybe something you would like to review.

[GreenWizard2015]

Ideally, I'd like to test the entire work cycle, but I'm not certain if the effort will be justified. I'll read through the documentation and then decide.

[psmgeelen]

Test on physical Device:

Processing uno_r4_wifi (platform: renesas-ra; board: uno_r4_wifi; framework: arduino)
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Verbose mode can be enabled via `-v, --verbose` option
CONFIGURATION: https://docs.platformio.org/page/boards/renesas-ra/uno_r4_wifi.html
PLATFORM: Renesas RA (1.3.0) > Arduino Uno R4 WiFi
HARDWARE: RA4M1 48MHz, 32KB RAM, 256KB Flash
DEBUG: Current (cmsis-dap) External (cmsis-dap, jlink)
PACKAGES: 
 - framework-arduinorenesas-uno @ 1.0.5 
 - tool-bossac @ 1.10901.0 (1.9.1) 
 - tool-jlink @ 1.78811.0 (7.88.11) 
 - tool-openocd @ 3.1200.0 (12.0) 
 - toolchain-gccarmnoneeabi @ 1.70201.0 (7.2.1)
LDF: Library Dependency Finder -> https://bit.ly/configure-pio-ldf
LDF Modes: Finder ~ chain, Compatibility ~ soft
Found 23 compatible libraries
Scanning dependencies...
Dependency Graph
|-- ArduinoJson @ 6.21.4
|-- aWOT @ 3.5.0
|-- RemoteControl
|-- WaterPumpController
Building in release mode
Checking size .pio/build/uno_r4_wifi/firmware.elf
Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
RAM:   [=         ]  13.8% (used 4528 bytes from 32768 bytes)
Flash: [==        ]  25.0% (used 65472 bytes from 262144 bytes)
Configuring upload protocol...
AVAILABLE: cmsis-dap, jlink, sam-ba
CURRENT: upload_protocol = sam-ba
Looking for upload port...
Auto-detected: /dev/ttyACM0
Forcing reset using 1200bps open/close on port /dev/ttyACM0
Uploading .pio/build/uno_r4_wifi/firmware.bin
Erase flash

Done in 0.002 seconds
Write 65472 bytes to flash (16 pages)

[                              ] 0% (0/16 pages)
[=                             ] 6% (1/16 pages)
[===                           ] 12% (2/16 pages)
[=====                         ] 18% (3/16 pages)
[=======                       ] 25% (4/16 pages)
[=========                     ] 31% (5/16 pages)
[===========                   ] 37% (6/16 pages)
[=============                 ] 43% (7/16 pages)
[===============               ] 50% (8/16 pages)
[================              ] 56% (9/16 pages)
[==================            ] 62% (10/16 pages)
[====================          ] 68% (11/16 pages)
[======================        ] 75% (12/16 pages)
[========================      ] 81% (13/16 pages)
[==========================    ] 87% (14/16 pages)
[============================  ] 93% (15/16 pages)
[==============================] 100% (16/16 pages)
Done in 4.086 seconds
--- Terminal on /dev/ttyACM0 | 9600 8-N-1
--- Available filters and text transformations: colorize, debug, default, direct, hexlify, log2file, nocontrol, printable, send_on_enter, time
--- More details at https://bit.ly/pio-monitor-filters
--- Quit: Ctrl+C | Menu: Ctrl+T | Help: Ctrl+T followed by Ctrl+H
Latest available version: 0.3.0
Please upgrade your firmware.

I cant find the device in my network, it seems that the device is not connecting to WiFi any more.. I am missing a lot of logging too...

From what I can tell, we have something go wrong in the RemoteControl.cpp in which we are printing Serial.prints from lines 57-62. But after the if statement, nothing seems to happen..

[psmgeelen]

Found the issue! Its line 61 with

while(true) delay(500);

Commenting that out, resolved the issue. The loop is probably infinite somehow.

Executing task: platformio run --target upload --target monitor --environment uno_r4_wifi 

Processing uno_r4_wifi (platform: renesas-ra; board: uno_r4_wifi; framework: arduino)
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Verbose mode can be enabled via `-v, --verbose` option
CONFIGURATION: https://docs.platformio.org/page/boards/renesas-ra/uno_r4_wifi.html
PLATFORM: Renesas RA (1.3.0) > Arduino Uno R4 WiFi
HARDWARE: RA4M1 48MHz, 32KB RAM, 256KB Flash
DEBUG: Current (cmsis-dap) External (cmsis-dap, jlink)
PACKAGES: 
 - framework-arduinorenesas-uno @ 1.0.5 
 - tool-bossac @ 1.10901.0 (1.9.1) 
 - tool-jlink @ 1.78811.0 (7.88.11) 
 - tool-openocd @ 3.1200.0 (12.0) 
 - toolchain-gccarmnoneeabi @ 1.70201.0 (7.2.1)
LDF: Library Dependency Finder -> https://bit.ly/configure-pio-ldf
LDF Modes: Finder ~ chain, Compatibility ~ soft
Found 23 compatible libraries
Scanning dependencies...
Dependency Graph
|-- ArduinoJson @ 6.21.4
|-- aWOT @ 3.5.0
|-- RemoteControl
|-- WaterPumpController
Building in release mode
Checking size .pio/build/uno_r4_wifi/firmware.elf
Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
RAM:   [=         ]  13.8% (used 4528 bytes from 32768 bytes)
Flash: [==        ]  25.0% (used 65456 bytes from 262144 bytes)
Configuring upload protocol...
AVAILABLE: cmsis-dap, jlink, sam-ba
CURRENT: upload_protocol = sam-ba
Looking for upload port...
Auto-detected: /dev/ttyACM0
Forcing reset using 1200bps open/close on port /dev/ttyACM0
Uploading .pio/build/uno_r4_wifi/firmware.bin
Erase flash

Done in 0.001 seconds
Write 65456 bytes to flash (16 pages)

[                              ] 0% (0/16 pages)
[=                             ] 6% (1/16 pages)
[===                           ] 12% (2/16 pages)
[=====                         ] 18% (3/16 pages)
[=======                       ] 25% (4/16 pages)
[=========                     ] 31% (5/16 pages)
[===========                   ] 37% (6/16 pages)
[=============                 ] 43% (7/16 pages)
[===============               ] 50% (8/16 pages)
[================              ] 56% (9/16 pages)
[==================            ] 62% (10/16 pages)
[====================          ] 68% (11/16 pages)
[======================        ] 75% (12/16 pages)
[========================      ] 81% (13/16 pages)
[==========================    ] 87% (14/16 pages)
[============================  ] 93% (15/16 pages)
[==============================] 100% (16/16 pages)
Done in 3.928 seconds
--- Terminal on /dev/ttyACM0 | 9600 8-N-1
--- Available filters and text transformations: colorize, debug, default, direct, hexlify, log2file, nocontrol, printable, send_on_enter, time
--- More details at https://bit.ly/pio-monitor-filters
--- Quit: Ctrl+C | Menu: Ctrl+T | Help: Ctrl+T followed by Ctrl+H
Latest available version: 0.3.0
Please upgrade your firmware.
Connecting to FamGeelen2.4Ghz
Atempt to connect: 1
.
Connection status: 3

Connected. IP: 192.168.178.60
SSID: FamGeelen2.4Ghz
BSSID: DC:39:6F:B8:B8:E3
signal strength (RSSI): -58
Encryption Type: 2
----------------------------------------------

[psmgeelen]

API works in practise too!

[psmgeelen]

My review:

• The code works!!
• I fixed a minor bug that I will be pushing after this message (infinite While loop)
• Removed unnecessary dependency: bblanchon/ArduinoJson@^6.21.4
• The flush method has been removed by @GreenWizard2015 as he considers this superfluous (not disagreeing)
• The threshold method has also been removed by @GreenWizard2015 , this is something that I don't understand why don't want to be able to control this via API.
• We should discuss what API methods are relevant for the 2 endpoints that we have. The endpoints are /pour_tea and possible (if we want to reconstitute the endpoint) the /set_threshold or /threshold endpoint

For some reference: https://www.freecodecamp.org/news/http-request-methods-explained/

[GreenWizard2015]

• I fixed a minor bug that I will be pushing after this message (infinite While loop)

Indeed, in the original code, there was only a firmware version check, WITHOUT halting operations. Personally, I find both versions of the code odd. If the firmware version does not match the library version, there's no guarantee of correct functioning, and it would be prudent to stop operation. On the other hand, this check seems pointless to me, since, for now, we definitely know whether the system is working or not. I would either completely remove this check or make it more stringent to ensure proper operation

• The threshold method has also been removed by @GreenWizard2015 , this is something that I don't understand why don't want to be able to control this via API.

For me, this device is like a satellite flying billions of kilometers away from Earth :) Its software needs to be as simple as possible.
Simplicity means eliminating all the unnecessary elements and minimizing points of failure. That's precisely why I removed the threshold setting. Need a different threshold? Then, please, personally reflash the device and check its functionality. Or better yet, implement additional logic in the external UI. The device itself should have the simplest possible code, one that operates 99.9999% of the time.
The possibility of configuring the threshold will only add unnecessary complexity to the code and potential risks. For example, what if, for some reason, the threshold is set too high and never activates? What if the pump is turned on and the threshold is changed immediately after? The user might think that the changes have taken effect, but they haven't! And let's not rely on the 'but Arduino is single-threaded and the user should understand that'. No, he shouldn't. We shouldn't depend on something that is implicit.

• We should discuss what API methods are relevant for the 2 endpoints that we have. The endpoints are /pour_tea and possible (if we want to reconstitute the endpoint) the /set_threshold or /threshold endpoint

I suggest the following changes:

• Revert /pour_tea to a POST request, but pass parameters as a query field, not as part of the URL.
• Add GET /status which would return, for example, the threshold hardcoded in the device.
• I'm considering an option with /pour_tea + /stop. /pour_tea should function as it does now, BUT with the ability to stop the process. This will complicate the code, but add convenience. For instance, you send "pour for 5 seconds", then see that the cup is overflowing and send a command to stop the process.

[psmgeelen]

Makes sense to me. Let me reply in kind:

• A UI will probably not be possible due to limited amount of memory
• I'm thinking reverting to POST is good idea too
• I am down with the status endpoint
• The intermittent stopping might not be possible. In the current implementation, the callback is the result of the of finishing the process of pouring. We would need something like an additional endpoint that produces a flag, that can be disabled via this endpoint, that will be continuously checked when the pouring_tea is actually pouring. Apart from whether/how C++ supports code-execution with variables that change in-flight, I hope that the processor is able to handle the concurrency. Arduinos can be very primitive.

[GreenWizard2015]

• A UI will probably not be possible due to limited amount of memory

I was planning to make the UI external. I have similar experience. I've created UI using ReactJS, where the user specifies the local IP of the device and interacts with it. The UI itself can be hosted in any way, for example, on Versel.

That's why I'm trying to make the API simple and flexible, moving all non-critical logic to the UI.

• The intermittent stopping might not be possible. In the current implementation, the callback is the result of the of finishing the process of pouring. We would need something like an additional endpoint that produces a flag, that can be disabled via this endpoint, that will be continuously checked when the pouring_tea is actually pouring. Apart from whether/how C++ supports code-execution with variables that change in-flight, I hope that the processor is able to handle the concurrency. Arduinos can be very primitive.

Yes, this will require complicating the code, a state machine, and will add edge cases. However, I believe the functionality being added is worth these efforts. I'll take care of it after the PR is accepted (unfortunately, I don't know how to properly start working before the PR is accepted).

[psmgeelen]

Branch is merged. Please make a feature/ui branch for your UI endeavors. :)