PR for review by JarikDem-Bot · Pull Request #6 · JarikDem-Bot/OCServo

JarikDem-Bot · 2025-01-23T16:32:06Z

PR for empty branch

BohdanBuinich · 2025-01-23T19:30:23Z

src/OCServo.h

+typedef enum e_SerialPortType {
+    SERIAL_HARDWARE,
+    SERIAL_SOFTWARE
+} SerialPortType;


The SerialPortType typedef could be unnecessary. Using enum SerialPortType directly is more standard and does not require the typedef keyword in modern C++.

Suggested change

typedef enum e_SerialPortType {

SERIAL_HARDWARE,

SERIAL_SOFTWARE

} SerialPortType;

/**

* @brief Type of serial port used by OCServo

*/

enum class SerialPortType {

Hardware,

Software

};

BohdanBuinich · 2025-01-23T19:33:42Z

src/OCServo.h

+enum OCSMode {
+    OCS_SERVO,
+    OCS_MOTOR
+};
+
+enum OCSResponseLevel {
+    OCS_RESPOND_READ_ONLY,
+    OCS_RESPOND_ALL
+};


Using enum class provides better type safety and prevents implicit conversions, ensuring that enumerators don’t conflict with other values or enumerations in the global namespace.

doc strings very useful add them to functions also

UPER case is usually used for defines, use camel case

Suggested change

enum OCSMode {

OCS_SERVO,

OCS_MOTOR

};

enum OCSResponseLevel {

OCS_RESPOND_READ_ONLY,

OCS_RESPOND_ALL

};

/**

* @brief Enumeration for the operating mode of OCServo

*/

enum class OCSMode {

Servo,

Motor

};

/**

* @brief Enumeration for the response level of OCServo

*/

enum class OCSResponseLevel {

RespondReadOnly,

RespondAll

};

BohdanBuinich · 2025-01-23T19:36:09Z

src/OCServo.h

+struct OCSResponse {
+    byte prefix[2];
+    byte id;
+    byte length;
+    byte instruction;
+    int numberOfParameters;
+    byte parameters[16];
+    byte checksum;
+};


Using fixed-length types like uint16_t or uint32_t ensures consistent and predictable behavior across different platforms, as the size of standard types (int, long) can vary. This makes code more portable and helps prevent subtle bugs caused by unexpected size differences. Please fix in the rest places.

Suggested change

struct OCSResponse {

byte prefix[2];

byte id;

byte length;

byte instruction;

int numberOfParameters;

byte parameters[16];

byte checksum;

};

/**

* @brief Structure representing a response from OCServo

*/

struct OCSResponse {

uint8_t prefix[2];

uint8_t id;

uint8_t length;

uint8_t instruction;

uint16_t parameterCount;

uint8_t parameters[16];

uint8_t checksum;

};

BohdanBuinich · 2025-01-23T19:38:47Z

src/OCServo.h

+        void ping();
+        OCSResponse ocsRead(byte address, byte length);
+        OCSResponse ocsWrite(byte address, int paramsNumber, byte *params);
+
+        OCSResponse setID(byte new_id);
+        void setBaudRate(long baudrate);
+        OCSResponse setMaxTorque(int torque);
+        OCSResponse setMode(int mode);
+        OCSResponse setGoalAngle(int angle, long timeMillis= 0);
+        OCSResponse setGoalPosition(int position, long timeMillis = 0);
+        OCSResponse setOperationTime(long timeMillis);
+        OCSResponse setResponseDelay(int delayMicros);
+        OCSResponse setResponseLevel(int level);
+        OCSResponse setMinAngle(int angle);
+        OCSResponse setMaxAngle(int angle);
+        OCSResponse setMaxVoltage(int voltage);
+        OCSResponse setMinVoltage(int voltage);
+        OCSResponse setOperationSpeed(long speed);
+
+        byte getID();
+        long getBaudRate();
+        int getMaxTorque();
+        int getMode();
+        int getGoalAngle();
+        int getGoalPosition();
+        long getOperationTime();
+        int getResponseDelay();
+        int getResponseLevel();
+        int getMinAngle();
+        int getMaxAngle();
+        int getMaxVoltage();
+        int getMinVoltage();
+        long getOperationSpeed();
+        bool reachedGoal();
+
+        void begin(long baudrate=1000000);
+        void printResponse(OCSResponse response);


Add brief comments to each method declaration would help users understand their purpose and intended usage.

Add const qualifiers to methods that don’t modify the class’s state, which improves safety and signals to the user that the function will not alter the object.

Suggested change

void ping();

OCSResponse ocsRead(byte address, byte length);

OCSResponse ocsWrite(byte address, int paramsNumber, byte *params);

OCSResponse setID(byte new_id);

void setBaudRate(long baudrate);

OCSResponse setMaxTorque(int torque);

OCSResponse setMode(int mode);

OCSResponse setGoalAngle(int angle, long timeMillis= 0);

OCSResponse setGoalPosition(int position, long timeMillis = 0);

OCSResponse setOperationTime(long timeMillis);

OCSResponse setResponseDelay(int delayMicros);

OCSResponse setResponseLevel(int level);

OCSResponse setMinAngle(int angle);

OCSResponse setMaxAngle(int angle);

OCSResponse setMaxVoltage(int voltage);

OCSResponse setMinVoltage(int voltage);

OCSResponse setOperationSpeed(long speed);

byte getID();

long getBaudRate();

int getMaxTorque();

int getMode();

int getGoalAngle();

int getGoalPosition();

long getOperationTime();

int getResponseDelay();

int getResponseLevel();

int getMinAngle();

int getMaxAngle();

int getMaxVoltage();

int getMinVoltage();

long getOperationSpeed();

bool reachedGoal();

void begin(long baudrate=1000000);

void printResponse(OCSResponse response);

void ping() const;

OCSResponse read(uint8_t address, uint8_t length) const;

OCSResponse write(uint8_t address, uint16_t parameterCount, const uint8_t *parameters) const;

OCSResponse setID(uint8_t newID);

void setBaudRate(uint32_t baudRate);

OCSResponse setMaxTorque(uint16_t torque);

OCSResponse setMode(OCSMode mode);

OCSResponse setGoalAngle(int16_t angle, uint32_t timeMillis = 0);

OCSResponse setGoalPosition(int16_t position, uint32_t timeMillis = 0);

OCSResponse setOperationTime(uint32_t timeMillis);

OCSResponse setResponseDelay(uint16_t delayMicros);

OCSResponse setResponseLevel(OCSResponseLevel level);

OCSResponse setMinAngle(int16_t angle);

OCSResponse setMaxAngle(int16_t angle);

OCSResponse setMaxVoltage(uint16_t voltage);

OCSResponse setMinVoltage(uint16_t voltage);

OCSResponse setOperationSpeed(uint32_t speed);

uint8_t getID() const;

uint32_t getBaudRate() const;

uint16_t getMaxTorque() const;

OCSMode getMode() const;

int16_t getGoalAngle() const;

int16_t getGoalPosition() const;

uint32_t getOperationTime() const;

uint16_t getResponseDelay() const;

OCSResponseLevel getResponseLevel() const;

int16_t getMinAngle() const;

int16_t getMaxAngle() const;

uint16_t getMaxVoltage() const;

uint16_t getMinVoltage() const;

uint32_t getOperationSpeed() const;

bool reachedGoal() const;

void begin(uint32_t baudRate = 1000000);

void printResponse(const OCSResponse &response) const;

BohdanBuinich · 2025-01-23T19:43:37Z

src/OCServo.h

+        byte getChecksum(byte *data, int size);
+        byte baudRateToByte(long baudrate);
+        long byteToBaudRate(byte value);
+        OCSResponse bytesToResponse(byte *data, int size);
+        OCSResponse readResponse();


Add brief comments to each method declaration would help users understand their purpose and intended usage.

Add const qualifiers to methods that don’t modify the class’s state, which improves safety and signals to the user that the function will not alter the object.

Suggested change

byte getChecksum(byte *data, int size);

byte baudRateToByte(long baudrate);

long byteToBaudRate(byte value);

OCSResponse bytesToResponse(byte *data, int size);

OCSResponse readResponse();

uint8_t calculateChecksum(const uint8_t *data, size_t size) const;

uint8_t baudRateToByte(uint32_t baudRate) const;

uint32_t byteToBaudRate(uint8_t value) const;

OCSResponse parseResponse(const uint8_t *data, size_t size) const;

OCSResponse readResponse() const;

BohdanBuinich · 2025-01-23T19:59:32Z

src/OCServo.cpp

+    return this->ocsWrite(OCS_OPERATION_TIME, 2, params);
+}
+
+OCSResponse OCServo::setResponseDelay(int delayMicros) {


do we really need to have int parameter, I think we can use uint16_t?

Suggested change

OCSResponse OCServo::setResponseDelay(int delayMicros) {

OCSResponse OCServo::setResponseDelay(uint16_t delayMicros) {

BohdanBuinich · 2025-01-23T20:00:14Z

src/OCServo.cpp

+OCSResponse OCServo::setResponseLevel(int level) {
+    byte value;
+
+    switch (level)
+    {
+        case OCS_RESPOND_READ_ONLY:
+            value = OCS_RESPONSE_READ;
+            break;
+        default:
+            // Fallthrough to OCS_RESPOND_ALL
+        case OCS_RESPOND_ALL:
+            value = OCS_RESPONSE_ALL;
+            break;
+    }
+
+    return this->ocsWrite(OCS_RESPONSE_LEVEL, 1, &value);
+}


MINOR:

Suggested change

OCSResponse OCServo::setResponseLevel(int level) {

byte value;

switch (level)

{

case OCS_RESPOND_READ_ONLY:

value = OCS_RESPONSE_READ;

break;

default:

// Fallthrough to OCS_RESPOND_ALL

case OCS_RESPOND_ALL:

value = OCS_RESPONSE_ALL;

break;

}

return this->ocsWrite(OCS_RESPONSE_LEVEL, 1, &value);

}

OCSResponse OCServo::setResponseLevel(OCSResponseLevel level) {

uint8_t value = (level == OCSResponseLevel::RespondReadOnly) ? OCS_RESPONSE_READ : OCS_RESPONSE_ALL;

return ocsWrite(OCS_RESPONSE_LEVEL, 1, &value);

}

BohdanBuinich · 2025-01-23T20:06:17Z

src/ROBSMotor.h

+/* INSTRUCTIONS */
+#define PING_INSTRUCTION    0x01
+#define READ_INSTRUCTION    0x02
+#define WRITE_INSTRUCTION   0x03
+
+/* ADDRESSES */
+#define OCS_SERVO_ID        0x05
+#define OCS_BAUD_RATE       0x06
+#define OCS_RESPONSE_DELAY  0x07
+#define OCS_RESPONSE_LEVEL  0x08
+#define OCS_MIN_ANGLE       0x09
+#define OCS_MAX_ANGLE       0x0B
+#define OCS_MAX_VOLTAGE     0x0E
+#define OCS_MIN_VOLTAGE     0x0F
+#define OCS_MAX_TORQUE      0x10
+#define OCS_RUNNING_MODE    0x23
+#define OCS_GOAL_POSITION   0x2A
+#define OCS_OPERATION_TIME  0x2C
+#define OCS_OPERATION_SPEED 0x2E
+#define OCS_OPERATING_SIGN  0x42


Add a consistent prefix (OCS_) and categorized macros by their purpose (INSTR_, ADDR_, MODE_, BAUDRATE_, etc.).

Add comments describing the purpose of each constant.

Suggested change

/* INSTRUCTIONS */

#define PING_INSTRUCTION 0x01

#define READ_INSTRUCTION 0x02

#define WRITE_INSTRUCTION 0x03

/* ADDRESSES */

#define OCS_SERVO_ID 0x05

#define OCS_BAUD_RATE 0x06

#define OCS_RESPONSE_DELAY 0x07

#define OCS_RESPONSE_LEVEL 0x08

#define OCS_MIN_ANGLE 0x09

#define OCS_MAX_ANGLE 0x0B

#define OCS_MAX_VOLTAGE 0x0E

#define OCS_MIN_VOLTAGE 0x0F

#define OCS_MAX_TORQUE 0x10

#define OCS_RUNNING_MODE 0x23

#define OCS_GOAL_POSITION 0x2A

#define OCS_OPERATION_TIME 0x2C

#define OCS_OPERATION_SPEED 0x2E

#define OCS_OPERATING_SIGN 0x42

/* INSTRUCTIONS */

#define OCS_INSTR_PING 0x01 // Sends a ping request

#define OCS_INSTR_READ 0x02 // Reads a parameter from the servo

#define OCS_INSTR_WRITE 0x03 // Writes a parameter to the servo

/* ADDRESSES */

#define OCS_ADDR_SERVO_ID 0x05 // Servo ID address

#define OCS_ADDR_BAUD_RATE 0x06 // Baud rate configuration

#define OCS_ADDR_RESPONSE_DELAY 0x07 // Response delay in microseconds

#define OCS_ADDR_RESPONSE_LEVEL 0x08 // Response level configuration

#define OCS_ADDR_MIN_ANGLE 0x09 // Minimum angle limit

#define OCS_ADDR_MAX_ANGLE 0x0B // Maximum angle limit

#define OCS_ADDR_MAX_VOLTAGE 0x0E // Maximum voltage limit

#define OCS_ADDR_MIN_VOLTAGE 0x0F // Minimum voltage limit

#define OCS_ADDR_MAX_TORQUE 0x10 // Maximum torque

#define OCS_ADDR_RUNNING_MODE 0x23 // Running mode (servo or motor)

#define OCS_ADDR_GOAL_POSITION 0x2A // Goal position

#define OCS_ADDR_OPERATION_TIME 0x2C // Operation time (in ms)

#define OCS_ADDR_OPERATION_SPEED 0x2E // Operation speed

#define OCS_ADDR_OPERATING_SIGN 0x42 // Operating sign

BohdanBuinich · 2025-01-23T20:11:30Z

src/OCServo.cpp

+    if (response.numberOfParameters != 2) {
+        return -1;
+    }
+    return ((response.parameters[1] << 8) | response.parameters[0] & 0x00FF);


make a define for it, it is easy to make typo mistake

#define BYTES_TO_UINT16(high, low) (((high) << 8) | ((low) & 0xFF))

Suggested change

return ((response.parameters[1] << 8) | response.parameters[0] & 0x00FF);

return BYTES_TO_UINT16(response.parameters[1], response.parameters[0]);

BohdanBuinich · 2025-01-23T20:12:27Z

src/OCServo.cpp

+    return result;
+}
+
+OCSResponse OCServo::bytesToResponse(byte *data, int size) {


size parameter not used?

BohdanBuinich · 2025-01-24T09:10:12Z

the configuration that is set in the motor is it recorded in permanent memory or temporary, for example, if you change the baudrate, then after reconnecting the power supply it will be the one you changed to?

JarikDem-Bot · 2025-01-24T10:30:39Z

the configuration that is set in the motor is it recorded in permanent memory or temporary, for example, if you change the baudrate, then after reconnecting the power supply it will be the one you changed to?

Hi! Thank you very much for reviewing my code!

This motor has both temporary and permanent memory. And according to datasheet, baud rate, as well as id and few other parameters, are stored in permanent memory.
But I've run into the issue, where after reconnecting motor's power supply, baud rate would return to the default 1M value.
At the same time, servo ID value worked as expected and stayed the same as I changed it to after reconnecting power.
I've spent a few days trying to fix it, but every time the result was the same, so I've decided to move on to other tasks

BohdanBuinich · 2025-01-24T10:37:17Z

And according to datasheet, baud rate, as well as id and few other parameters, are stored in permanent memory.

Ok, good, can we do automatic configuration for the current baudrate, the user doesn't need to guess what the current baudrate is, your api needs to find it and establish a connection. What do you think?

JarikDem-Bot · 2025-01-24T10:52:01Z

Yeah, this is great idea, and it will definitely improve user experience. Thank you!

To implement it, I would:

send ping command to the motor.
If there is no answer after timeout, then I'll change baud rate and send ping command again.
And if I get a correct response, then it is the current baud rate value we will use in our program.

What do you think about this solution?

BohdanBuinich · 2025-01-24T10:54:22Z

What do you think about this solution?

Perfect, I was thinking the same thing

Commit

b6ebf6f

BohdanBuinich reviewed Jan 23, 2025

View reviewed changes

	OCSResponse OCServo::setResponseDelay(int delayMicros) {
	OCSResponse OCServo::setResponseDelay(uint16_t delayMicros) {

	return ((response.parameters[1] << 8) \| response.parameters[0] & 0x00FF);
	return BYTES_TO_UINT16(response.parameters[1], response.parameters[0]);

Conversation

JarikDem-Bot commented Jan 23, 2025

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!

BohdanBuinich commented Jan 24, 2025

Uh oh!

JarikDem-Bot commented Jan 24, 2025

Uh oh!

BohdanBuinich commented Jan 24, 2025

Uh oh!

JarikDem-Bot commented Jan 24, 2025

Uh oh!

BohdanBuinich commented Jan 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants