CAN library and .ino test code upload #5

m4r14g · 2024-12-27T22:33:48Z

No description provided.

luca-byte

Great job 🎯

As we saw in the meeting the code is working, which is the most important thing.

I would like to just leave some generic comments regarding the next steps and the needed improvements.

At the moment this library (in my opinion) is not ready to go into the source code (src) because is using many magic numbers instead of using the "final approach". As I see it we can either 1. put this in the test folder or 2. restructure it as if the other libraries are already present. Both this options have their pros and cons, but in any case I would gladly listen to your input on the matter.
Moreover the code has many stylistic problems (multiple blank lines, unaligned braces,...), which in codebases can be problematic, so I would suggest you to run your code through a formatter. If you have any questions about it, ask me.
In the end I noticed that the code uses slightly different approaches each time it does something similar, which impacts negatively the readability (for example the feedback value sometimes is obtained before setting up the CAN address, sometimes after).

⚠️ Consider that these are very small issues but it is the work of the reviewers to point these out to obtain a as-good-as-possible codebase. I really consider this to be a very good approach to obtain a working CAN interface and the testing-driven development is something very positive. I suggest you to work on points 2 and 3 while we can also discuss about the approach for point 1.

Obviously, feel free to reach out if you need anything or if something is not clear.

luca-byte · 2024-12-27T22:35:37Z

ESP32LowLevel/CAN.cpp

@@ -0,0 +1,196 @@
+#include "CAN.h"
+
+#include <cstring>


I think this is not needed. Could you please verify it?

luca-byte · 2024-12-27T22:39:01Z

ESP32LowLevel/CAN.cpp

+
+#include <cstring>
+
+#define CAN_ID    0x11  // this shouldn't be here in the end but specified in the makefile


Suggested change

#define CAN_ID 0x11 // this shouldn't be here in the end but specified in the makefile

As you noted this should not be here. As for tests you can define temporarily #define MOD_HEAD in mod_config.h but this should not be pushed to production

luca-byte · 2024-12-27T22:39:49Z

ESP32LowLevel/CAN.cpp

+// I
+void CAN::init() {
+// configure CAN
+   CAN0.setCANPins((gpio_num_t)36, (gpio_num_t)35);


This PINs can go to the definition file IMHO

luca-byte · 2024-12-27T22:40:23Z

ESP32LowLevel/CAN.cpp

+void CAN::init() {
+// configure CAN
+   CAN0.setCANPins((gpio_num_t)36, (gpio_num_t)35);
+   CAN0.begin(125000);


This (the bitrate) can be a parameter in the CAN.h class

luca-byte · 2024-12-27T22:41:44Z

ESP32LowLevel/CAN.cpp

+// configure CAN
+   CAN0.setCANPins((gpio_num_t)36, (gpio_num_t)35);
+   CAN0.begin(125000);
+   CAN0.watchFor(); // use this to filter for specific IDs


Question: at the moment the CAN is not filtering messages? If this is the case, it is problematic because it could enact on commands meant for other modules

luca-byte · 2024-12-27T22:55:00Z

ESP32LowLevel/CAN.cpp

+bool CAN::receiveMotorSetpoint() {
+    // take the first four bytes from the array and and put them in a float
+        float leftSpeed, rightSpeed;
+        memcpy(&leftSpeed, canMsg.data.uint8, 4); // Copia i primi 4 byte per leftSpeed


Comments should be in english

luca-byte · 2024-12-27T22:56:22Z

ESP32LowLevel/CAN.cpp

+    // DEBUG: print frame
+      Serial.println("I'm receiving...");
+          Serial.print(canMsg.id, HEX);
+          if (canMsg.extended) {
+            Serial.print(" X ");}
+          else {
+            Serial.print(" S ");  } 
+            Serial.print(canMsg.length, DEC);
+            for (int i = 0; i < canMsg.length; i++) {
+            Serial.print(canMsg.data.byte[i], HEX);
+            Serial.print(" ");}
+            Serial.println();


This will go to the Debug class, but is ok

luca-byte · 2024-12-27T23:01:29Z

ESP32LowLevel/CAN.cpp

+    if (type == MOTOR_SETPOINT_ID) {
+      receiveMotorSetpoint();
+    } 
+    else if (type == END_EFFECTOR_PITCH_SETPOINT_ID || 
+             type == END_EFFECTOR_HEAD_PITCH_SETPOINT_ID || 
+             type == END_EFFECTOR_HEAD_ROLL_SETPOINT_ID) {
+      receiveEndEffectorSetpoint(type);
+    } 
+    else if (type == JOINT_YAW_SETPOINT_ID) {
+      receiveJointSetpoint();
+    }
+
+    return true;
+  }


what about a switch-case?

Suggested change

if (type == MOTOR_SETPOINT_ID) {

receiveMotorSetpoint();

}

else if (type == END_EFFECTOR_PITCH_SETPOINT_ID ||

type == END_EFFECTOR_HEAD_PITCH_SETPOINT_ID ||

type == END_EFFECTOR_HEAD_ROLL_SETPOINT_ID) {

receiveEndEffectorSetpoint(type);

}

else if (type == JOINT_YAW_SETPOINT_ID) {

receiveJointSetpoint();

}

return true;

}

switch (type) {

case MOTOR_SETPOINT_ID:

receiveMotorSetpoint();

break;

case END_EFFECTOR_PITCH_SETPOINT_ID:

case END_EFFECTOR_HEAD_PITCH_SETPOINT_ID:

case END_EFFECTOR_HEAD_ROLL_SETPOINT_ID:

receiveEndEffectorSetpoint(type);

break;

case JOINT_YAW_SETPOINT_ID:

receiveJointSetpoint();

break;

}

return true;

}

luca-byte · 2024-12-27T23:04:30Z

ESP32LowLevel/CAN.cpp

+    // motorTrLeft.stop();
+    // motorTrRight.stop();


luca-byte · 2024-12-27T23:05:21Z

ESP32LowLevel/CAN.h

+/* 
+   Using https://github.com/collin80/esp32_can
+   which requires https://github.com/collin80/can_common
+   Install both by downloading the ZIP file from GitHub.
+
+*/
+
+#include <esp32_can.h>


If these are file we need to copy (not installable using the Arduino IDE) I think that we can copy them directly in the codebase in this folder

luca-byte · 2024-12-28T09:17:51Z

About the discussion (1.), I realized that there is also another option which I believe to be cleaner and which would make the CAN code agnostic w.r.t. the sensor implementation and methods, using lambda functions. Essentially the methods which provide feedbacks and which receive setpoints will have as parameters a lambda function which will be called and provides the implementation

An example for the joint feedback follows:

#include <functional>

class CAN {
public:
    void sendJointFeedback(std::function<float()> getAngle);
    // altre funzioni e membri...
};

void CAN::sendJointFeedback(std::function<float()> getAngle) {
    float angle = getAngle();
    canMsg.id = 0; 
    canMsg.id = CAN_ID | (JOINT_YAW_FEEDBACK_ID << 16);     
    canMsg.length = 4;  
    canMsg.rtr = 0;    
    canMsg.extended = 1; 
    memcpy(canMsg.data.uint32, &angle, 4);
    CAN0.sendFrame(canMsg);
}

// Esempio di utilizzo
AbsoluteEncoder encoderYaw;
CAN can;

can.sendJointFeedback([&encoderYaw]() { return encoderYaw.readAngle(); });

If you are interested in considering this approach, it can be tested by passing a constant function

can.sendJointFeedback([]() { return 5.0f; });

CAN library and .ino test code

cc4f670

m4r14g requested a review from luca-byte December 27, 2024 22:33

luca-byte requested changes Dec 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CAN library and .ino test code upload #5

CAN library and .ino test code upload #5

m4r14g commented Dec 27, 2024

luca-byte left a comment

luca-byte Dec 27, 2024

luca-byte Dec 27, 2024

luca-byte Dec 27, 2024

luca-byte Dec 27, 2024 •

edited

Loading

luca-byte Dec 27, 2024

luca-byte Dec 27, 2024

luca-byte Dec 27, 2024

luca-byte Dec 27, 2024

luca-byte Dec 27, 2024

luca-byte Dec 27, 2024

luca-byte commented Dec 28, 2024


		#include <cstring>

		#define CAN_ID 0x11 // this shouldn't be here in the end but specified in the makefile

CAN library and .ino test code upload #5

Are you sure you want to change the base?

CAN library and .ino test code upload #5

Conversation

m4r14g commented Dec 27, 2024

luca-byte 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

luca-byte Dec 27, 2024 • edited Loading

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

luca-byte commented Dec 28, 2024

luca-byte Dec 27, 2024 •

edited

Loading