a performance improvement for scanning #262

veerwang · 2025-06-04T10:27:39Z

By simultaneously driving the optimization adjustments of the X and Y axes, a performance improvement of approximately 5% was achieved.

ianohara

I left a bunch of comments. Some ask for clarification, some for fixes.

At least 2 things we definitely need to do:

Fix the changes to CephlaStage to respect AbstractStage
Decide whether or not this is okay to roll out as is, since it will break all customers until they do a firmware update.

@hongquanli for 2. - how do we want to handle that? If we merge this all customers will break until they do a firmware update (if they update their code to at or post merge of this PR).

ianohara · 2025-06-23T16:28:42Z

firmware/octopi_firmware_v2/main_controller_teensy41/main_controller_teensy41.ino

 // byte[3]: how many micro steps - lower 8 bits

-static const int CMD_LENGTH = 8;
+static const int CMD_LENGTH = 24;


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.

I haven't thought much about it; I'm just leaving room for future situations. OK, I would change to 10.

The command length must be adjusted in multiples of 4 bytes. @ianohara

ianohara · 2025-06-23T16:37:15Z

firmware/octopi_firmware_v2/main_controller_teensy41/TMC4361A.h

  int target_tolerance;
  int pid_tolerance;

+  int ramp_mode;	


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 set tmc4361a->ramp_mode = TMC4361_RAMP_SSHAP in the init.

I know we set the ramp mode in our .py implementation, but you generally don't want to assume other pieces of code will do the right thing.

OK, I agree with you.

@veerwang can you implement the suggested change?

OK, I would do it.

ianohara · 2025-06-23T16:38:18Z

firmware/octopi_firmware_v2/main_controller_teensy41/main_controller_teensy41.ino

  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);
+    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);


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?

The knob is designed to increase with clockwise rotation and decrease with counterclockwise rotation. This modification aims to align with conventional intuition.

ianohara · 2025-06-23T16:43:09Z

firmware/octopi_firmware_v2/main_controller_teensy41/main_controller_teensy41.ino

+            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)


Can you elaborate on the comment above? What code is the host code in this case?

Also, we have AXIS_W defined - can you use that?

And I don't understand why we map AXIS_W to AXIS_THETA here. Can you explain that?

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 5 are AXIS_W or some other use of the number 5). Bare numbers are only sortof okay if they are true universal constants (as in, they never change throughout the entire universe).

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:

static const int AXIS_X = 0; static const int AXIS_Y = 1; static const int AXIS_Z = 2; static const int AXIS_THETA = 3; static const int AXES_XY = 4; static const int AXIS_W = 5;

Here are the python defines:

class AXIS: X = 0 Y = 1 Z = 2 THETA = 3 XY = 4 W = 5

Sure, I would change the code to easy to understand.

ianohara · 2025-06-23T16:46:39Z

firmware/octopi_firmware_v2/main_controller_teensy41/main_controller_teensy41.ino

+              axis = 3;
+
+            // guard code
+            if (axis > STAGE_AXES)


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?

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.

This just fails silently, though. If someone tries to use this command with axis > STAGE_AXES, how do they know that they didn't actually set ramp mode on the axis?

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!

ianohara · 2025-06-23T17:03:14Z

firmware/octopi_firmware_v2/main_controller_teensy41/main_controller_teensy41.ino

    {
      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;


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."

ianohara · 2025-06-23T17:08:41Z

software/control/core/multi_point_worker.py


        self.coordinates_pd = pd.concat([self.coordinates_pd, new_row], ignore_index=True)

    def move_to_coordinate(self, coordinate_mm):


Maybe there's a reason I'm missing here, but this refactor duplicates code. Instead, I probably would do:

def move_to_coordinate(self, coordinate_mm): x_mm = coordinate_mm[0] y_mm = coordinate_mm[1] have_z = len(coordinate_mm) >= 3 self.stage.move_xy_to(x_mm, y_mm, blocking=not have_z) if have_z >= 3: self.move_to_z_level(z_mm) self._sleep(SCAN_STABILIZATION_TIME_MS / 1000)

This:

Does not duplicate the x_mm, y_mm code.

Doesn't have two move_xy_to calls

Also, since SCAN_STABILIZATION_TIME_MS_Y is no longer used for just Y in your code, we should re-name it. This is to follow the "principle of least surprise" - it'd be really surprising to someone in the future to change SCAN_STABILIZATION_TIME_MS_Y and see that all the axis move stabilization times change (not just Y).

OK， I would copy the code.

ianohara · 2025-06-23T17:10:45Z

software/control/microcontroller.py

        cmd[5] = payload & 0xFF
        self.send_command(cmd)

+    def move_xy_to_usteps(self, xusteps, yusteps):


It looks like we also added a MOVE_XY - we should add that here so users of the Microcontroller know it exists. Or remove it from the firmware.

OK, I will temporarily remove MOVE_XY from the firmware.

ianohara · 2025-06-23T17:14:04Z

software/squid/stage/cephla.py

                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):


We have an AbstractStage interface now. This means the codebase outside of squid/stage can only use methods on the AbstractStage class, and any class that implements AbstractStage must implement all the @abstractmethod on it. As is, this breaks the PriorStage and the AbstractStage interface.

To do this change properly, we should:

Add an @abstractmethod called move_xy_to to AbstractStage.

Implement move_xy_to on all the stage implementations in squid/stage (right now, just the cephla and prior stages).

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!

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

Or I just add an empty function in prior_stages?

ianohara

Other than adding the PriorStage definition, this looks good (assuming it has been tested on all the Cephla stage hardware it will need to run on).

@hongquanli we still need to decide what we want to do in terms of managing firmware updates. Once this merges, users will need to do a firmware update if they update their repos.

ianohara · 2025-09-08T16:27:45Z

firmware/octopi_firmware_v2/main_controller_teensy41/main_controller_teensy41.ino

+              axis = 3;
+
+            // guard code
+            if (axis > STAGE_AXES)


This just fails silently, though. If someone tries to use this command with axis > STAGE_AXES, how do they know that they didn't actually set ramp mode on the axis?

ianohara · 2025-09-08T16:40:45Z

software/squid/stage/prior.py

            threading.Thread(target=self.wait_for_stop, daemon=True).start()

+    def move_xy_to(self, x_abs_mm: float, y_abs_mm: float, blocking: bool = True):
+        pass


For now, I think you can just make this:

def move_xy_to(self, x_abs_mm: float, y_abs_mm: float, blocking: bool = True): self.move_x_to(x_abs_mm, blocking) self.move_y_to(y_abs_mm, blocking)

ianohara · 2025-09-08T16:49:29Z

firmware/octopi_firmware_v2/main_controller_teensy41/main_controller_teensy41.ino

+              axis = 3;
+
+            // guard code
+            if (axis > STAGE_AXES)


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!

veerwang added 10 commits June 4, 2025 16:39

add move_xy_to API into firmware & change the CMD length from 8 to 24

53e3bbe

switch Z value tuning direction

7f696ed

cancel waiting logic for turn on illumination

28f0684

Update information as soon as the axis arrives at the destination

860725f

Merge branch 'master' into pr-speedup

9eb55fa

cancel useless code

d3d0b32

reformat code

c901147

change S-ramp to Trapezoidal ramp of Z axis

c6fbcca

make sramp mode to configure method

b7a9ce2

reformat code

1c1f885

ianohara requested changes Jun 23, 2025

View reviewed changes

ianohara mentioned this pull request Jun 23, 2025

firmware: check and send position updates every 5ms, not 10ms #211

Closed

veerwang added 8 commits July 15, 2025 13:40

restore to CMD_LENGTH to 10

5cb40c7

initialize the ramp_mode with TMC4361A_RAMP_SSHAP value

c427919

make the code more easy to understand

3a1aa64

add comment

8ebcf2c

slight update

2f93b68

remove MOVE_XY temporarily

6a3c3ac

modify code into AbstractStage class

b5ee8a5

reformat

b1e393e

veerwang requested a review from ianohara July 16, 2025 00:40

veerwang added 5 commits July 16, 2025 16:47

bug fixed for CMD_LENGTH change from 24 to 10

10e8ebe

bug fixed after self-testing

a9acc9c

bug fix

7d53cc3

bug fixed for CMD_LENGTH change from 10 to 12

68869a5

Merge branch 'Cephla-Lab:master' into pr-speedup

dd613bd

veerwang requested a review from hongquanli July 29, 2025 00:38

ianohara approved these changes Sep 8, 2025

View reviewed changes


		self.coordinates_pd = pd.concat([self.coordinates_pd, new_row], ignore_index=True)

		def move_to_coordinate(self, coordinate_mm):

a performance improvement for scanning #262

Are you sure you want to change the base?

a performance improvement for scanning #262

Uh oh!

Conversation

veerwang commented Jun 4, 2025

Uh oh!

ianohara left a comment

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!

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!

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!

ianohara left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants