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

motors in a separate process working version #8

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

Conversation

EmanPaoli
Copy link
Member

No description provided.

Copy link
Member

@vilim vilim left a comment

Choose a reason for hiding this comment

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

Looks good, needs just a few small adjustments to make it more pythonic and readable

def move_motors(self):

for axis in self.motors.keys():
package = self.get_last_entry(self.input_queues[axis])
Copy link
Member

Choose a reason for hiding this comment

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

this is a bit mysterious, I suggest that get_last_entry (in a factored-out version) throws the Empty exception and then you can have the much more compact:

try:
   mov_amount, mov_type = self.get_last_entry(self.input_queues[axis])
   if mov_type == MovementType.relative:
   ...
except empty:
   return

value = motor.home_pos
self.input_queue = input_queue
self.output_queue = output_queue
self.mov_type = MovementType(False)
Copy link
Member

Choose a reason for hiding this comment

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

you would rather use the enum as MovementType.relative (the point of using Enums is that this mapping true, false or integers is arbitrary)


def update_external(self, new_val):
self.slider.pos = new_val
self.spin_val_actual_pos.setValue(new_val)
self.slider.update()


def get_next_entry(queue):
Copy link
Member

Choose a reason for hiding this comment

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

this can be factored out in thecolonel (which can also get a new culinary name)

self.motors[axis].move_abs(mov_value)

@staticmethod
def get_last_entry(queue):
Copy link
Member

Choose a reason for hiding this comment

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

this appears in many places, I suggest to factor it out

package = self.get_last_entry(self.input_queues[axis])
if package:
mov_value = package[0]
mov_type = package[1].name
Copy link
Member

Choose a reason for hiding this comment

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

again, this goes against the spirit of using Enums

empty_queue = True

if empty_queue is False:
if mov_type == "relative":
Copy link
Member

Choose a reason for hiding this comment

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

the point of using Enums is to avoid string comparisons

encoding=self.encoding, timeout=10
)
self.start_session()

@staticmethod
def find_axis(axes):
Copy link
Member

Choose a reason for hiding this comment

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

axes can be an Enum and this function just a dictionary lookup


class PreMotor:
@staticmethod
def query(self):
Copy link
Member

Choose a reason for hiding this comment

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

what is this?

def update_values(self, val):
self.spin_val_desired_pos.setValue(val)
self.sig_changed.emit(val)
while True:
Copy link
Member

Choose a reason for hiding this comment

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

this again is the queue quering pattern that should be in a separate, common function


def update_external(self, new_val):
self.slider.pos = new_val
self.spin_val_actual_pos.setValue(new_val)
self.slider.update()


def get_next_entry(queue):
out = None
while out is None:
Copy link
Member

Choose a reason for hiding this comment

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

again the pattern

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.

2 participants