Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

brushed board firmware #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

brushed board firmware #2

wants to merge 1 commit into from

Conversation

mn297
Copy link
Contributor

@mn297 mn297 commented Apr 13, 2024

No description provided.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mn297 since we are not using torque control, maybe we should clean up that portion of the code prior to merging to main?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be better to keep it for future-proofing

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably remove this file no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@@ -0,0 +1,469 @@
#include "common.h"
#if USE_ROS_FIRMWARE == 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#if USE_ROS_FIRMWARE == 1
#ifdef USE_ROS_FIRMWARE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also move this preprocessor macro below the includes

ros::NodeHandle nh;
float arm_brushed_setpoint_ps[3] = {0, 0, 0};
float arm_brushed_angle_ps[3] = {0, 0, 0};
void brushed_board_ros_loop();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of method definitions that should be added to a header for ease of reading.

void brushed_board_homing();
void brushed_board_ros_loop();

void lim1ISR();
Copy link

@amotaouakkil123 amotaouakkil123 Apr 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add these functions declarations and definitions in common.h and a new common.cpp? This would separate between the helper functions and the loops.

int cur1_voltage_buffer_idx = 0;
int cur2_voltage_buffer_idx = 0;
int cur3_voltage_buffer_idx = 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, if there are a handful of function definitions and variable definitions, it would be fine putting them at the top of a .cpp file. Since there a lot of them, for better readability it would help putting them in a separate file.


void brushed_board_ros_loop()
{
#if DEBUG_PRINT == 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few of these #if <SOME_MACRO> == 1. I would suggest

Suggested change
#if DEBUG_PRINT == 1
#ifdef DEBUG_PRINT

float enc2_angle_ps = mot2.get_current_angle_ps();
float enc2_setpoint = mot2.get_target_angle_ps();

// float enc2_quad_enc_pos = mot2._encoder->_encoder->read();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this dead code?

HWSERIAL.printf("enc2_quad_enc_pos: %8.4f, enc2_angle_single: %8.4f, enc2_angle_es: %8.4f, enc2_angle_ps: %8.4f, enc2_setpoint: %8.4f, ",
enc2_quad_enc_pos, enc2_angle_single, enc2_angle_es, enc2_angle_ps, enc2_setpoint);

// float enc1_rev = mot1._encoder->_encoder->getRevolution();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants