Skip to content

Make picomotor current position data streams 64-bit#439

Merged
RemiSoummer merged 2 commits into
developfrom
force_int_type_in_picomotor
Jun 1, 2026
Merged

Make picomotor current position data streams 64-bit#439
RemiSoummer merged 2 commits into
developfrom
force_int_type_in_picomotor

Conversation

@ivalaginja

@ivalaginja ivalaginja commented May 7, 2026

Copy link
Copy Markdown
Collaborator

Bug found while working on capsule.

Python's int() function returns a 64-bit integer on the capsule Linux machine, which trips up the service when it gets fed into a data stream that expects a 32-bit integer. This PR forces it to 32-bit upon submission into said data stream.

Tested on hardware and works.

@ivalaginja ivalaginja requested a review from RemiSoummer May 7, 2026 09:32
@ivalaginja ivalaginja self-assigned this May 7, 2026
@ivalaginja ivalaginja added the bugfix Fixing of a bug label May 7, 2026
@ivalaginja

Copy link
Copy Markdown
Collaborator Author

@ehpor thinking about it, does it make a difference if I force the data stream to be created for 64-bit integers, or if I force all integers on that data stream to be 32-bit? Any general thoughts that would motivate one or the other?

@ivalaginja ivalaginja requested a review from ehpor May 7, 2026 09:34
@ehpor

ehpor commented May 14, 2026

Copy link
Copy Markdown
Collaborator

@ivalaginja I don't remember why I used 32-bit numbers. Maybe to limit the maximum number of steps? I see nothing in the code that signifies that the hardware needs an explicit cap at 32 bit. So making the data stream 64bit would be my solution, if you can test that on hardware first.

@ivalaginja ivalaginja force-pushed the force_int_type_in_picomotor branch from 8dcabc1 to e9ca8c6 Compare May 24, 2026 16:09
@ivalaginja

Copy link
Copy Markdown
Collaborator Author

@ehpor tested on the Linux machine with 64-bit and this works.

@ivalaginja ivalaginja changed the title Force 32-bit integer for data stream that expects it Make picomotor data stream 64-bit May 24, 2026
@ivalaginja ivalaginja changed the title Make picomotor data stream 64-bit Make picomotor current position data streams 64-bit May 24, 2026
@RemiSoummer

Copy link
Copy Markdown
Collaborator

Tested on HiCAT hardware and works well, including sending a float giving a ValueError and not crashing.

@RemiSoummer RemiSoummer force-pushed the force_int_type_in_picomotor branch from e9ca8c6 to 42adff4 Compare June 1, 2026 20:18

@RemiSoummer RemiSoummer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

all good and tested

@RemiSoummer RemiSoummer enabled auto-merge June 1, 2026 20:22
@RemiSoummer RemiSoummer merged commit f8ae2d3 into develop Jun 1, 2026
6 checks passed
@RemiSoummer RemiSoummer deleted the force_int_type_in_picomotor branch June 1, 2026 20:30
@raphaelpclt

raphaelpclt commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

I'm just seeing this now, and just as a sidenote while looking at #232. We did enforce integer casting in the proxy as well because picomotors fail if we don't cast the integers into integers (yes - twice). So even if it's working, it's now more of a code style discrepancy since int32 are used there

new_position = np.int32(current_position + distance)

@ivalaginja

Copy link
Copy Markdown
Collaborator Author

I'm just seeing this now, and just as a sidenote while looking at #232. We did enforce integer casting in the proxy as well because picomotors fail if we don't cast the integers into integers (yes - twice). So even if it's working, it's now more of a code style discrepancy since int32 are used there

new_position = np.int32(current_position + distance)

Oh, you're right! I also just realized that I never adjusted the int type for the current position data stream in the simulated service.
I opened #443 to fix this. Do you think you could review that @raphaelpclt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fixing of a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants