Conversation
|
New code |
| #define PAGE_SIZE_BYTES 256 | ||
| #define XIP_BASE_ADDRESS 0x10000000 | ||
| #define ROBOT_MAGIC_NUMBER 1 | ||
|
|
There was a problem hiding this comment.
These #defines is one of the main reasons I mentioned splitting nonvolatile stuff to a separate file; the rest of the actuator code doesn't really care about them and it leads to low file cohesion
| bool is_moving; | ||
| int32_t target_pos_continuous; | ||
| bool target_pos_reached; | ||
|
|
There was a problem hiding this comment.
Why do you define these global variables here? They don't seem to be used anywhere. Same is done down below (lines 139-140)
| extern servo_t claw_grip; | ||
| extern servo_t claw_rack; | ||
| extern servo_t *servos[NUM_SERVOS]; | ||
|
|
There was a problem hiding this comment.
Declaring these extern is fine for now while testing, but try to restrict them just to the .c file for final production. Also, I don't see any reason you need to include actuator.h here. Really all you need is the servo_t struct definition, found in hiwonder_driver.h
|
|
||
| if (abs(servo->absolute_pos - servo->target_pos_continuous) < STOPPING_TOLERANCE) { | ||
| // Stop servo | ||
| uint8_t param_buf[MAX_PACKET_SIZE]; // Is MAX_PACKET_SIZE necessary |
There was a problem hiding this comment.
No, MAX_PACKET_SIZE isn't strictly required but it makes dealing with the buffer easier since you can put it on the stack instead of having to malloc it (because it's a fixed size)
|
|
||
| if (err != SERVO_READ_OK) { | ||
| LOG_WARN("Servo read error"); | ||
| } |
There was a problem hiding this comment.
Why multiple checks for SERVO_READ_OK? This second one will never run
| } | ||
|
|
||
| const int32_t ROLLOVER_THRESHOLD = 500; // 32768 | ||
| const int32_t FULL_RANGE = 1400; |
There was a problem hiding this comment.
Please never declare constants this way, even while testing. If they must exist, place them in a #define at the top of the file. Hiding the constants inside the functions makes it much harder to find them when you need to change them later, constants being the things that are most frequently modified after the code is put into production
| servo_read_continuous(servos[i]); | ||
| } | ||
| */ | ||
| servo_read_continuous(&claw_grip); |
There was a problem hiding this comment.
This is fine during testing, but this should get moved out of main for production
lilboat/HBridgeBoard/src/main.c
Outdated
| write_to_flash(&servo_config); | ||
| */ | ||
|
|
||
| servo_continuous_move_deg(&claw_grip, 360, 200); |
There was a problem hiding this comment.
Try to avoid testing like this unless you're getting really weird issues. It's better to make a more versatile test platform with ROS inputs
| printf("Read failed with error code: %d\n", err); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Try to avoid placing functions like these in main as well
|
A couple general notes:
|
… and skip unnecessary calls when the servo isn't moving
2 participants
No description provided.