Task 3, libSMCE console #51

Ryllee · 2021-11-23T14:48:33Z

We are aware that the code is in need of refactoring, but we are not sure how we should go about it and also what to prioritize.

Added SMCE_Client, a terminal interface for libSMCE.

Implemented a way to handle more then one input, to make it possible for example sending message

NOT WORKING! KNOWN BUG.

Move SMCE_Client to its own folder, created a CMakeLists that installs SMCE_Client.exe and copys the SMCE.dll from the path of enviroment variable SMCE_ROOT.

Fixed bugg with recompile causing thread to crash. Added termcolor library to set color for uart messages. Added mute for uart messages.

Added functionality to set Arduino root folder Added functionality to set SMCE_RESOURCE at runtime

Added so incoming uart messages can be written to a file instead of console. Set at start by setting the start argument -u or --file to true.

codecov · 2021-11-23T15:35:07Z

Codecov Report

Merging #51 (79ed7ca) into master (47c8e01) will increase coverage by 23.18%.
The diff coverage is 5.26%.

❗ Current head 79ed7ca differs from pull request most recent head 100b84e. Consider uploading reports for the commit 100b84e to get more accurate results

@@             Coverage Diff             @@
##           master      #51       +/-   ##
===========================================
+ Coverage   40.08%   63.26%   +23.18%     
===========================================
  Files          33       82       +49     
  Lines        1497     2986     +1489     
===========================================
+ Hits          600     1889     +1289     
- Misses        897     1097      +200

Impacted Files	Coverage Δ
client/CMakeLists.txt	`0.00% <0.00%> (ø)`
CMakeLists.txt	`94.21% <100.00%> (ø)`
src/SMCE/Board.cpp	`86.76% <0.00%> (-1.38%)`	⬇️
src/SMCE/Toolchain.cpp	`67.44% <0.00%> (-0.40%)`	⬇️
src/Ardrivo/MQTT.cpp	`6.57% <0.00%> (-0.28%)`	⬇️
src/SMCE/Sketch.cpp	`100.00% <0.00%> (ø)`
include/SMCE/Board.hpp	`100.00% <0.00%> (ø)`
include/SMCE/BoardView.hpp	`94.44% <0.00%> (ø)`
include/SMCE/Toolchain.hpp	`66.66% <0.00%> (ø)`
CMake/Modules/BindGen.cmake	`96.00% <0.00%> (ø)`
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47c8e01...100b84e. Read the comment docs.

AeroStun

This code is barely readable; reformat it (ClangFormat is your friend).
Also the general conditional complexity in main tends to be overwhelming; try factorizing it out using lookup tables, functions, etc...

AeroStun · 2021-11-23T15:29:13Z

CMakeLists.txt

@@ -129,7 +129,7 @@ target_sources (HostUDD PRIVATE
 )


-add_library (objSMCE OBJECT)


Superfluous change

AeroStun · 2021-11-23T15:29:26Z

CMakeLists.txt

@@ -153,6 +153,7 @@ target_sources (objSMCE PRIVATE
    include/SMCE/SketchConf.hpp
    include/SMCE/internal/BoardDeviceSpecification.hpp
 )
+


Superfluous change

AeroStun · 2021-11-23T15:30:18Z

client/CMakeLists.txt

+        )
+FetchContent_MakeAvailable(Termcolor Lyra)
+
+include_directories(${PROJECT_SOURCE_DIR}/src)


Never use global include_directories; see target_include_directories for a pure alternative

AeroStun · 2021-11-23T15:31:46Z

client/CMakeLists.txt

+target_include_directories (${PROJECT_NAME} PUBLIC
+        "${termcolor_SOURCE_DIR}/include/termcolor"
+        "${lyra_SOURCE_DIR}/include/lyra"
+        )


I would be incredibly surprised that the termcolor::termcolor and bfg::Lyra targets don't already provide those directories in their interface properties

AeroStun · 2021-11-23T15:33:05Z

client/README.md

+To be able to build SMCE_client, the enviroment variable SMCE_ROOT needs to point to a current installed release of libSMCE.
+These can be found under releases on git. Or it can be build directly from the source code with the commands below.
+These should be ran in the libSMCE folder. 
+```shell
+cmake -S . -B build/
+cmake --build build/
+cmake --build build/ --config Release
+cmake --install build/ --prefix <path to installation folder>


These instructions are both wrong and misplaced; this README is in a subfolder of the libSMCE project, so it stands to reason that readers have already read the top-level README

AeroStun · 2021-11-23T15:50:07Z

client/SMCE_Client.cpp

+
+        }else if (input.starts_with("-ra ")){                                                                                    
+            bool read = true;                                                                                                    
+            int index_pin = stoi(input.substr(3));                                                                               


const-incorrect

AeroStun · 2021-11-23T15:50:37Z

client/SMCE_Client.cpp

+
+        }else if (input.starts_with("-rd ")){
+            bool read = true;
+            int index_pin = stoi(input.substr(3));


const-incorrect

AeroStun · 2021-11-23T15:51:05Z

client/src/UartToFile.cpp

@@ -0,0 +1,28 @@
+#include <UartToFile.hpp>
+#include <ctime>


Use <chrono>'s stuff

AeroStun · 2021-11-23T15:51:31Z

client/src/UartToFile.cpp

+#include <fstream>
+#include <iostream>
+
+using namespace std;


If you are trying to give me a heart attack, this is definitely the way to go

AeroStun · 2021-11-23T15:52:28Z

client/src/UartToFile.cpp

+int uart_to_file(std::string message, std::string path){
+    ofstream file;
+    file.open(path, std::ios_base::app);
+    file << get_time() +": " +message + "\n";
+    file.close();
+    return 0;
+}


Opening/closing a file every time you need to write to it should be a definition of inefficiency.

RuthgerD

Focus on parting everything into components, just constantly ask yourself, "how would I test this", "could someone reuse this code as a library" and stuff like that.

RuthgerD · 2021-11-23T14:51:38Z

client/README.md

+- Sketch path: Relative or absolute path to the sketch to run
+
+### Start arguments
+-f,--fqbn <fqbn>                    -> <fqbn> = Fully qualified board name 


just a note in general, fqbn is basically deprecated, so I would default this parameter

RuthgerD · 2021-11-23T14:53:01Z

client/src/UartToFile.cpp

+std::string get_time(){
+    time_t currentTime;
+    struct tm *localTime;
+
+    time( &currentTime );
+    localTime = localtime( &currentTime );
+
+    int hour   = localTime->tm_hour;
+    int min    = localTime->tm_min;
+    int sec    = localTime->tm_sec;
+    return(to_string(hour)+":"+to_string(min)+":"+to_string(sec));
+}


this use the way better library:
https://en.cppreference.com/w/cpp/chrono#Clocks

RuthgerD · 2021-11-23T14:53:40Z

client/src/UartToFile.cpp

+#include <fstream>
+#include <iostream>
+
+using namespace std;


using std is a bad practice

RuthgerD · 2021-11-23T14:55:27Z

client/src/UartToFile.cpp

+int uart_to_file(std::string message, std::string path){
+    ofstream file;
+    file.open(path, std::ios_base::app);
+    file << get_time() +": " +message + "\n";
+    file.close();
+    return 0;
+}


this is a highly specifically named module and a highly specifically named function name even though all it does write to a file. I would generalize this, also, this is one of the cases where you might want to keep the file open for the duration of the program as constantly opening a file is not very nice.

RuthgerD · 2021-11-23T14:56:44Z

client/SMCE_Client.cpp

+void print_help(const char* argv0){                                                                                              
+    std::cout << "Usage: " << argv0 << " <fully-qualified-board-name> <path-to-sketch>" << std::endl;                            
+}                                                                                                                                


stuff like this can just be in the main function

RuthgerD · 2021-11-23T15:53:16Z

client/SMCE_Client.cpp

+            print_menu();
+
+        }else if (input == "-p"){ //pause or resume                                                                               
+            if(!suspended){                                                                                                      


better read the state from the board instead of keeping track of it yourself

RuthgerD · 2021-11-23T15:54:25Z

client/SMCE_Client.cpp

+        }else if (input == "-rc"){ //recompile
+            std::cout << "Recompiling.." << std::endl; 
+            t_lock.lock(); // aquire lock before recompiling, so uart_listener is forced to wait                                                                                                                                                                        
+            compile_and_start(sketch,toolchain,board,arduino_root_dir);


a function like this just makes things opaque imo, why not have a compile function and a start function that you just calls after each other? they dont need to be composed.

RuthgerD · 2021-11-23T15:54:51Z

client/SMCE_Client.cpp

+            std::cout << "Complete" << std::endl;
+
+        }else if (input.starts_with("-m ")){ //send message on uart                                                              
+                std::string message = input.substr(3);                                                                           


needless clone

RuthgerD · 2021-11-23T15:55:50Z

client/SMCE_Client.cpp

+            if(!apin.exists()){                                                                                                  
+                std::cout << "Pin does not exist!" << std::endl;                                                                 
+                write=false;                                                                                                     
+            }                                                                                                                    
+            if(!apin.can_write()){                                                                                               
+                std::cout << "Can't write to pin!" << std::endl;                                                                 
+                write=false;                                                                                                     
+            }                                                                                                                    
+            if(write){                                                                                                           
+                apin.write(value);                                                                                               
+            }


if () else if() else { ::write }

RuthgerD · 2021-11-23T15:56:50Z

client/SMCE_Client.cpp

+                if(value == 0){                                                                                                  
+                apin.write(false);                                                                                               
+                }else if(value == 1){                                                                                            
+                    apin.write(true);                                                                                            
+                }else{                                                                                                           
+                    std::cout << "Value must be 0 or 1 for digital pins" << std::endl;                                           
+                }                                                                                                                


tbh apin.write(value > 0), it works like this internally as well

RuthgerD · 2021-11-23T16:22:43Z

client/SMCE_Client.cpp

+static std::atomic_bool mute_uart = false;
+static std::mutex t_lock;
+//Listen to the uart input from board, writes what it read to terminal                                                           
+void uart_listener(smce::VirtualUart uart,bool file_write,std::string path){                                                                                      


yeah as discussed, taking the VirtualUart by value and reusing it across recompilations is completely wrong and likely reads invalid memory

Focused on the smaller problems from the pull request review. Still has several bugs / changes that needs to be done.

AeroStun · 2021-11-26T08:36:39Z

client/SMCE_Client.cpp

-        }else if (input.starts_with("-rd ")){
-            bool read = true;
-            int index_pin = stoi(input.substr(3));
+            }else if (!pin.can_read()) {


Just because the board cannot read does not mean the user should not be able to see the value

AeroStun · 2021-11-26T08:37:06Z

client/SMCE_Client.cpp

-            }
-            if(!pin.can_read()){
+            }else if (!pin.can_read()) {
                std::cout << "Can't read from pin!" << std::endl;


Same as above

AeroStun · 2021-11-26T08:38:12Z

client/SMCE_Client.cpp

                std::cout << "Value from pin " << index_pin << " is " << pin.read() << std::endl;
            }

-        }else if (input == "-q"){ //power off and quit
+        } else if (input == "-q") { // power off and quit
            std::cout << "Quitting..." << std::endl;
            run_threads = false;
            board.stop();


Board::stop may fail

Ryllee and others added 16 commits October 9, 2021 15:22

Added SMCE_Client

cbffdf6

Added SMCE_Client, a terminal interface for libSMCE.

Can now handle more then 1 input

95a9a9e

Implemented a way to handle more then one input, to make it possible for example sending message

Implemented messaging through UART

ed37707

NOT WORKING! KNOWN BUG.

Refactored SMCE_Client to it own folder

7da05fd

Move SMCE_Client to its own folder, created a CMakeLists that installs SMCE_Client.exe and copys the SMCE.dll from the path of enviroment variable SMCE_ROOT.

Update CMakeLists.txt

af112b4

Added copy SMCE.dll post_build in cmake

023a2ae

Added GPIO functionality to libSMCE client

4bed0b0

Added functionality to read values on gpio pins

ecdb59f

Fixed typo

84390c5

Known bug: Issues with thread

b5d3520

Recompile, mute and color added

68eb00b

Fixed bugg with recompile causing thread to crash. Added termcolor library to set color for uart messages. Added mute for uart messages.

Added a README for SMCE_Client

2f26c6f

Added CMake installation of Lyra and Termcolor

b957ff5

Added start arguments with lyra

b2ba11c

Added functionality to set Arduino root folder Added functionality to set SMCE_RESOURCE at runtime

Added uart to file write

117473b

Added so incoming uart messages can be written to a file instead of console. Set at start by setting the start argument -u or --file to true.

Global variables changes and typos

79ed7ca

AeroStun requested changes Nov 23, 2021

View reviewed changes

RuthgerD reviewed Nov 23, 2021

View reviewed changes

Fixed typos and small refactoring

2a29d24

Focused on the smaller problems from the pull request review. Still has several bugs / changes that needs to be done.

AeroStun reviewed Nov 26, 2021

View reviewed changes

Removed ctime and used chrono instead

100b84e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task 3, libSMCE console #51

Task 3, libSMCE console #51

Ryllee commented Nov 23, 2021

codecov bot commented Nov 23, 2021 •

edited

Loading

AeroStun left a comment

AeroStun Nov 23, 2021

AeroStun Nov 23, 2021

AeroStun Nov 23, 2021

AeroStun Nov 23, 2021

AeroStun Nov 23, 2021

AeroStun Nov 23, 2021

AeroStun Nov 23, 2021

AeroStun Nov 23, 2021

AeroStun Nov 23, 2021

AeroStun Nov 23, 2021

RuthgerD left a comment

RuthgerD Nov 23, 2021

RuthgerD Nov 23, 2021

RuthgerD Nov 23, 2021

RuthgerD Nov 23, 2021

RuthgerD Nov 23, 2021

RuthgerD Nov 23, 2021

RuthgerD Nov 23, 2021

RuthgerD Nov 23, 2021

RuthgerD Nov 23, 2021

RuthgerD Nov 23, 2021

RuthgerD Nov 23, 2021

AeroStun Nov 26, 2021

AeroStun Nov 26, 2021

AeroStun Nov 26, 2021

		@@ -129,7 +129,7 @@ target_sources (HostUDD PRIVATE
		)


		add_library (objSMCE OBJECT)

Task 3, libSMCE console #51

Are you sure you want to change the base?

Task 3, libSMCE console #51

Conversation

Ryllee commented Nov 23, 2021

codecov bot commented Nov 23, 2021 • edited Loading

Codecov Report

AeroStun 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RuthgerD 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

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

codecov bot commented Nov 23, 2021 •

edited

Loading