a performance improvement for scanning #262
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,7 @@ | |
| // byte[2]: how many micro steps - upper 8 bits | ||
| // byte[3]: how many micro steps - lower 8 bits | ||
|
|
||
| static const int CMD_LENGTH = 24; | ||
|
Collaborator
There was a problem hiding this comment. I can't figure out why this needs to go up to 24 instead of 10 (we need 2 extra bytes for the MOVE*_XY, but I don't see why we need 11-24). It's probably okay, but it does mean we send 14 more bytes per command than we absolutely need to. I think that's only ~8 us more per command at 2 MHz, but if we don't need it we might as well not waste the time.
Contributor
Author
There was a problem hiding this comment. I haven't thought much about it; I'm just leaving room for future situations. OK, I would change to 10.
Contributor
Author
There was a problem hiding this comment. The command length must be adjusted in multiples of 4 bytes. @ianohara |
||
| static const int MSG_LENGTH = 24; | ||
| byte buffer_rx[512]; | ||
| byte buffer_tx[MSG_LENGTH]; | ||
|
|
@@ -67,6 +67,9 @@ static const int SET_PID_ARGUMENTS = 29; | |
| static const int SEND_HARDWARE_TRIGGER = 30; | ||
| static const int SET_STROBE_DELAY = 31; | ||
| static const int SET_AXIS_DISABLE_ENABLE = 32; | ||
| static const int MOVE_XY = 33; | ||
| static const int MOVETO_XY = 34; | ||
| static const int SET_SRAMP_MOD = 36; | ||
| static const int SET_PIN_LEVEL = 41; | ||
| static const int INITFILTERWHEEL = 253; | ||
| static const int INITIALIZE = 254; | ||
|
|
@@ -301,12 +304,11 @@ uint16_t home_safety_margin[4] = {4, 4, 4, 4}; | |
| // IntervalTimer does not work on teensy with SPI, the below lines are to be removed | ||
| static const int TIMER_PERIOD = 500; // in us | ||
| volatile int counter_send_pos_update = 0; | ||
|
|
||
| static const int interval_send_pos_update = 10000; // in us | ||
| elapsedMicros us_since_last_pos_update; | ||
|
|
||
| static const int interval_check_position = 5000; // in us | ||
| elapsedMicros us_since_last_check_position; | ||
|
|
||
| static const int interval_send_joystick_update = 30000; // in us | ||
|
|
@@ -356,7 +358,7 @@ void onJoystickPacketReceived(const uint8_t* buffer, size_t size) | |
| } | ||
| else | ||
| { | ||
| focusPosition = focusPosition - (int32_t(uint32_t(buffer[0]) << 24 | uint32_t(buffer[1]) << 16 | uint32_t(buffer[2]) << 8 | uint32_t(buffer[3])) - focuswheel_pos); | ||
|
Collaborator
There was a problem hiding this comment. I can't figure out why we switched the sign here. Can you help me understand how this is related to scanning performance, and what the sign switch does?
Contributor
Author
There was a problem hiding this comment. The knob is designed to increase with clockwise rotation and decrease with counterclockwise rotation. This modification aims to align with conventional intuition. |
||
| focuswheel_pos = int32_t(uint32_t(buffer[0]) << 24 | uint32_t(buffer[1]) << 16 | uint32_t(buffer[2]) << 8 | uint32_t(buffer[3])); | ||
| } | ||
|
|
||
|
|
@@ -911,6 +913,21 @@ void loop() { | |
| } | ||
| break; | ||
| } | ||
| case MOVETO_XY: | ||
| { | ||
| long x_absolute_position = int32_t(uint32_t(buffer_rx[2]) << 24 | uint32_t(buffer_rx[3]) << 16 | uint32_t(buffer_rx[4]) << 8 | uint32_t(buffer_rx[5])); | ||
| X_direction = sgn(x_absolute_position - tmc4361A_currentPosition(&tmc4361[x])); | ||
| X_commanded_target_position = x_absolute_position; | ||
| if (tmc4361A_moveTo(&tmc4361[x], X_commanded_target_position) == 0) | ||
| X_commanded_movement_in_progress = true; | ||
| long y_absolute_position = int32_t(uint32_t(buffer_rx[6]) << 24 | uint32_t(buffer_rx[7]) << 16 | uint32_t(buffer_rx[8]) << 8 | uint32_t(buffer_rx[9])); | ||
| Y_direction = sgn(y_absolute_position - tmc4361A_currentPosition(&tmc4361[y])); | ||
| Y_commanded_target_position = y_absolute_position; | ||
| if (tmc4361A_moveTo(&tmc4361[y], Y_commanded_target_position) == 0) | ||
| Y_commanded_movement_in_progress = true; | ||
| mcu_cmd_execution_in_progress = true; | ||
| break; | ||
| } | ||
| case MOVETO_Z: | ||
| { | ||
| long absolute_position = int32_t(uint32_t(buffer_rx[2]) << 24 | uint32_t(buffer_rx[3]) << 16 | uint32_t(buffer_rx[4]) << 8 | uint32_t(buffer_rx[5])); | ||
|
|
@@ -938,6 +955,21 @@ void loop() { | |
| } | ||
| break; | ||
| } | ||
| case SET_SRAMP_MOD: | ||
| { | ||
| uint8_t axis = buffer_rx[2]; | ||
| uint8_t mode = buffer_rx[3]; | ||
| // map to w axis, in the host w axis index is 5 | ||
| if (axis == 5) | ||
|
Collaborator
There was a problem hiding this comment. Can you elaborate on the comment above? What code is the host code in this case? Also, we have And I don't understand why we map In general - try to avoid using bare numbers like this. Instead, use constants with clear names. This helps make the code more clear, and helps in refactoring later (For instance, if we need to change the axis number for AXIS_W in the future, it's easy to change the AXIS_W define but hard to go and figure out which inline
Collaborator
There was a problem hiding this comment. It looks like the python axes match the defines here, so I'm really not sure what the "map to w axis, in host w axis is 5" comment means. Here are the firmware defines: Here are the python defines:
Contributor
Author
There was a problem hiding this comment. Sure, I would change the code to easy to understand. |
||
| axis = 3; | ||
|
|
||
| // guard code | ||
| if (axis > STAGE_AXES) | ||
|
Collaborator
There was a problem hiding this comment. Why is it only okay to change the ramp mode for X, Y, and Z? It isn't okay to change them for THETA and W?
Contributor
Author
There was a problem hiding this comment. For safety reasons, I’ve only tested along the X, Y, and Z axes so far, so I’m keeping it in safe mode for now.
Collaborator
There was a problem hiding this comment. This just fails silently, though. If someone tries to use this command with
Collaborator
There was a problem hiding this comment. It looks like we let all other commands fail silently, so I guess we can keep this. We really need to introduce a mechanism for communicating command failures, though. Failing silently shouldn't be an option! |
||
| return; | ||
|
|
||
| tmc4361[axis].ramp_mode = mode; | ||
| break; | ||
| } | ||
| case SET_LIM: | ||
| { | ||
| switch (buffer_rx[2]) | ||
|
|
@@ -2181,9 +2213,9 @@ void loop() { | |
| buffer_tx[18] &= ~ (1 << BIT_POS_JOYSTICK_BUTTON); // clear the joystick button bit | ||
| buffer_tx[18] = buffer_tx[18] | joystick_button_pressed << BIT_POS_JOYSTICK_BUTTON; | ||
|
|
||
| // Calculate and fill out the checksum. NOTE: This must be after all other buffer_tx modifications are done! | ||
| uint8_t checksum = crc8ccitt(buffer_tx, MSG_LENGTH - 1); | ||
| buffer_tx[MSG_LENGTH - 1] = checksum; | ||
|
|
||
| if(!DEBUG_MODE) | ||
| SerialUSB.write(buffer_tx,MSG_LENGTH); | ||
|
|
@@ -2201,8 +2233,6 @@ void loop() { | |
| SerialUSB.print(", PG:"); | ||
| SerialUSB.println(digitalRead(pin_PG)); | ||
| } | ||
| } | ||
|
|
||
| // keep checking position process at suitable frequence | ||
|
|
@@ -2213,22 +2243,26 @@ void loop() { | |
| { | ||
| X_commanded_movement_in_progress = false; | ||
| mcu_cmd_execution_in_progress = false || Y_commanded_movement_in_progress || Z_commanded_movement_in_progress || W_commanded_movement_in_progress; | ||
| us_since_last_check_position = interval_check_position + 1; | ||
|
Collaborator
There was a problem hiding this comment. A comment here to explain this might be helpful. Something like "// It's important that we check positions next time around because <explain why ...>, so force the elapsed time to be larger than our interval." |
||
| } | ||
| if (Y_commanded_movement_in_progress && tmc4361A_currentPosition(&tmc4361[y]) == Y_commanded_target_position && !is_homing_Y && !tmc4361A_isRunning(&tmc4361[y], stage_PID_enabled[y])) | ||
| { | ||
| Y_commanded_movement_in_progress = false; | ||
| mcu_cmd_execution_in_progress = false || X_commanded_movement_in_progress || Z_commanded_movement_in_progress || W_commanded_movement_in_progress; | ||
| us_since_last_check_position = interval_check_position + 1; | ||
| } | ||
| if (Z_commanded_movement_in_progress && tmc4361A_currentPosition(&tmc4361[z]) == Z_commanded_target_position && !is_homing_Z && !tmc4361A_isRunning(&tmc4361[z], stage_PID_enabled[z])) | ||
| { | ||
| Z_commanded_movement_in_progress = false; | ||
| mcu_cmd_execution_in_progress = false || X_commanded_movement_in_progress || Y_commanded_movement_in_progress || W_commanded_movement_in_progress; | ||
| us_since_last_check_position = interval_check_position + 1; | ||
| } | ||
| if (enable_filterwheel == true) { | ||
| if (W_commanded_movement_in_progress && tmc4361A_currentPosition(&tmc4361[w]) == W_commanded_target_position && !is_homing_W && !tmc4361A_isRunning(&tmc4361[w], stage_PID_enabled[w])) | ||
| { | ||
| W_commanded_movement_in_progress = false; | ||
| mcu_cmd_execution_in_progress = false || X_commanded_movement_in_progress || Y_commanded_movement_in_progress || Z_commanded_movement_in_progress; | ||
| us_since_last_check_position = interval_check_position + 1; | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -2270,28 +2304,6 @@ void loop() { | |
| } | ||
| } | ||
|
|
||
| /***************************************************************************************************/ | ||
| /********************************************* utils *********************************************/ | ||
| /***************************************************************************************************/ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,6 +107,18 @@ def move_y_to(self, abs_mm: float, blocking: bool = True): | |
| self._calc_move_timeout(abs_mm - self.get_pos().y_mm, self.get_config().Y_AXIS.MAX_SPEED) | ||
| ) | ||
|
|
||
| def move_xy_to(self, x_abs_mm: float, y_abs_mm: float, blocking: bool = True): | ||
|
Collaborator
There was a problem hiding this comment. We have an To do this change properly, we should:
Test, or find someone that can help you test, the new code on all the stages we support. We should not merge this before doing all of the above since it will break some customers!
Contributor
Author
There was a problem hiding this comment. OK， I would add an @AbstractMethod called move_xy_to to AbstractStage. Would you please ask somebody do the implementation of move_xy_to in to prior_stages？ @hongquanli
Contributor
Author
There was a problem hiding this comment. Or I just add an empty function in prior_stages? |
||
| self._microcontroller.move_xy_to_usteps( | ||
| self._config.X_AXIS.convert_real_units_to_ustep(x_abs_mm), | ||
| self._config.Y_AXIS.convert_real_units_to_ustep(y_abs_mm), | ||
| ) | ||
|
|
||
| if blocking: | ||
| x_timeout = self._calc_move_timeout(x_abs_mm - self.get_pos().x_mm, self.get_config().X_AXIS.MAX_SPEED) | ||
| y_timeout = self._calc_move_timeout(y_abs_mm - self.get_pos().y_mm, self.get_config().Y_AXIS.MAX_SPEED) | ||
|
|
||
| self._microcontroller.wait_till_operation_is_completed(max(x_timeout, y_timeout)) | ||
|
|
||
| def move_z_to(self, abs_mm: float, blocking: bool = True): | ||
| # From Hongquan, we want the z axis to rest on the "up" (wrt gravity) direction of gravity. So if we | ||
| # are moving in the negative (down) z direction, we need to move past our mark a bit then | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we don't initialize this in
tmc4361A_init, which means I think we default it to 0. To preserve the original behavior it'd be good to settmc4361a->ramp_mode = TMC4361_RAMP_SSHAPin the init.I know we set the ramp mode in our
.pyimplementation, but you generally don't want to assume other pieces of code will do the right thing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I agree with you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@veerwang can you implement the suggested change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I would do it.