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

Extend APT Motor Controller to set / get home parameters #337

Merged
merged 3 commits into from
Mar 25, 2022

Conversation

trappitsch
Copy link
Contributor

For APT Motor Controllers, homing parameters can be set. This was so far not included in ik. Now it is, and tested (w/ pytests and with a "PRM1-Z8" stage).

Small addition to the overall monster APT driver.

homing velocity to be changed.

.. note:: When setting the quantity, pass `None` to values
that you want to leave unchanged (see example below).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this would be neat for the setter. For my current application, I don't care about the velocity or which way around the limit switch works, I just want to leave that at default and only want to adjust the offset. This way, I can pass None for values I don't want to change and set parameters that I want.

NotImplemented,
NotImplemented,
u.Quantity(42941.66, u.sec / u.deg),
u.Quantity(14.66, u.sec ** 2 / u.deg),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird thing here: Distances are incorporated as "encouder counts per degree", however, velocity and acceleration according to the Thorlabs manual table as scaling factors... It's also not counts per second, so leaving velocity units away.

Copy link
Contributor

Choose a reason for hiding this comment

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

😂

@coveralls
Copy link

coveralls commented Mar 24, 2022

Coverage Status

Coverage increased (+0.002%) to 99.486% when pulling 086e5bc on trappitsch:apt_ext into 8894ca0 on Galvant:main.

req_units = (1 / self.scale_factors[1]).units
try:
velocity = int(
(velocity.to(req_units) * self.scale_factors[1]).magnitude
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would have liked to include this in a nicer fashion and simply divide by the scaling factor. However, somehow pint seems to not notice dimensionless units when doing math, e.g.:

>>> a = 1*u.m
>>> b = 1*u.cm
>>> print(a/b)
1.0 meter / centimeter

If you do the same with identical units, it comes out as dimensionless. Need to check more, might be a pint issue? The workaround here is functional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out, this can actually be done in pint and I should have just read the manual... Solution is:

c=a/b
c.ito_reduced_units()

This makes it dimensionless.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I was going to mention today that these libraries usually have a function that needs to be called to actually reduce the units to base SI

# replace values that are `None`
if None in values:
set_params = self.home_parameters
for it in range(len(values)):
Copy link
Contributor

Choose a reason for hiding this comment

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

😉

values = [x if x else y for x, y in zip(values, set_params)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then i also don't have to convert potential tuples to lists first

home_dir, lim_sw, velocity, offset = values
# check validity of unitful values
if not isinstance(velocity, u.Quantity):
velocity = int(velocity)
Copy link
Contributor

Choose a reason for hiding this comment

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

If its NOT an instance of u.Quantity, cast it to an int?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm just getting tripped up by the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the comment is indeed just wrong. pretty self-explanatory i guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually: moving it to int anyway when the package is created, where it makes more sense. so there's no reason to do it here - neither with the offset.

Copy link
Contributor

@scasagrande scasagrande left a comment

Choose a reason for hiding this comment

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

lgtm!

@scasagrande scasagrande merged commit bd4fc9f into instrumentkit:main Mar 25, 2022
@trappitsch trappitsch deleted the apt_ext branch March 25, 2022 15:49
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