Threading #38 #42 #59

fergusL · 2019-10-16T09:04:03Z

This PR implements non-blocking versions of the goto_az(), .calibrate_dome_encoder_counts() and .find_home() methods (#38).

I've also fixed the simulated mode so that the triggering of gpio pins is handled by timed threads, as opposed to just letting various wait_for_active() calls to time out as was the case previously (#42).

This PR also relates to #52 and #32.

AnthonyHorton

Looks good. If the abort is working in simulation I think we're reading to start testing by moving the real thing.

AnthonyHorton · 2019-10-16T13:38:08Z

domehunter/dome_control.py

-        # degrees_per_tick for the tolerance
-        self.az_position_tolerance = max(
-            self.az_position_tolerance, self.degrees_per_tick)
+        self._az_position_tolerance = Angle(az_position_tolerance * u.deg)


I think you still should be checking whether the azimuth tolerance is less than one encoder tick, because it really shouldn't be. Probably serious enough to raise a ValueError.

As raise in the error instead of just using the degrees_per_tick and sweeping it under the rug? Or should we still change the tolerance to be the same as degrees_per_tick but log some kind of warning message?

domehunter/dome_control.py

AnthonyHorton · 2019-10-16T13:43:25Z

domehunter/dome_control.py

+        self._move_event = threading.Event()
+        # creating a threading simulated_rotation event, to indicate when a
+        # simulated rotation thread is running for testing mode calibration
+        self._simulated_rotation_event = threading.Event()


Do you really need to use a special Event for this?

Well, when I start the calibration thread I'm already setting the move_event flag. If I want to be able to simulate rotations for testing mode, I need a way of knowing when simulated rotations are done.

AnthonyHorton · 2019-10-16T14:11:52Z

domehunter/dome_control.py

@@ -279,6 +289,11 @@ def dome_in_motion(self):
        logger.debug(f'Dome in motion: {dome_motion}')
        return dome_motion

+    @property
+    def movement_thread_active(self):


I'd be tempted to call this is_moving as it's more intuitive, & more in keeping with the general principle of naming things after meaning and not implementation details. The only things giving me second thoughts about that are that the dome can take up to a few seconds to come to a halt after being told to stop, and that the dome could be moving because someone is in the dome pressing the handset buttons. Maybe move_active instead?

That thought about the handset raises another issue. If someone does move the dome manually then the Pi is likely to lose track of where the dome actually is. It will see the encoder pulsing, but it has not way of telling which way the dome is moving. It would just assume the dome is moving in the direction that it last told it to, which might be wrong. What should happen instead is if the encoder starts registering pulses more than a few seconds after the Pi has told it to stop the Pi should just flag itself as 'unhomed', and refuse to move the dome again until eithe rehomed or synced.

Another point is that I already had a .dome_in_motion property that returns True when the rotation relay is active. So kind of two things doing the same thing? Maybe I don't need the movement event at all?
Agree with the manual control problem, will make this an issue

AnthonyHorton · 2019-10-16T14:19:03Z

domehunter/dome_control.py

+        while self._move_event.is_set():
+            pass


It's a shame there isn't an equivalent of the .wait() method for waiting until an Event is not set...

You should put a timeout in here, just in case a move thread gets itself in such a mess it fails to respond to the abort and clear the ._move_event.

The way abort is set up currently, it only triggers _thread_condition() to break out of the while True: loop. All of the actual actions are then carried out from within _thread_condition(), such as turning off the rotation relay, clearing the _move_event etc etc.
If we include time out in the actual abort() method, do we also want to run through the same process? Otherwise all the timeout would do is clear the abort_event flag?

domehunter/dome_control.py

wtgee

Looks good, the only concern would be what seem to be some extra threads within the calibrate method.

wtgee · 2019-10-16T20:19:23Z

domehunter/dome_control.py

+    def movement_thread_active(self):
+        """Return true if a movement thread is running"""
+        return self._move_event.is_set()


I would also suggest a more intuitive name that doesn't include thread. Depends on what logic you want, but maybe is_moving.

Might be right, we also already have a .dome_in_motion property that is based off the rotation_relay state. I'm no longer sure if we even need this threading event.

domehunter/dome_control.py

wtgee · 2019-10-16T20:35:16Z

domehunter/dome_control.py

+        first_rotation = threading.Thread(target=self._simulate_rotation)
+        first_rotation.start()


Do you need these? I don't see them anywhere else and this is calling a thread from within a thread.

So there is the calibration monitoring thread, that is checking for the right number of calibration rotations, abort events and timeouts. This needs to be making those checks regularly. For testing mode I also need to simulate the rotations, so they need to be run in a non-blocking thread.
So to begin I start an initial rotation before starting the while loop and include a clause within that loop that checks if the previous simulated rotation thread has completed. If the number of desired is rotations has been met the monitor thread will terminate, otherwise it will start another simulated rotation in its own thread.
Maybe there is a better way of doing this but the idea was that the thread monitoring calibration needs run separately to the thread simulating the rotation. I could have set the rotation threads up with a delay from within the main calibrate_dome_encoder_counts() method but you'd have to specify the timing of when each simulated rotation would start. I just thought that seemed messy/could lead to over lapping threads (a new rotation thread starts before the last is done etc).

domehunter/dome_control.py

wtgee

Check the comment about the az_position_tolerance but I think this is good otherwise. The simulated calibration threads still seem a bit odd to me but since it's just for the simulation and that is working via the tests then I assume all good.

domehunter/dome_control.py

wtgee · 2019-10-17T09:59:54Z

domehunter/dome_control.py

+        delta_az = self.current_direction * raw_delta_az
+        logger.debug(f'Delta_az is {delta_az}, tolerance window is {self.az_position_tolerance}')
+        return delta_az <= self.az_position_tolerance


Won't multiplying it by the current_direction lead to a negative number with Direction.CCW? If so then the delta_az will always be less than the az_position_tolerance.

nah multiplying by the Direction enum ensures that delta az is always positive. So if we are at an az of 170 and want to go to an az of 90, the raw_delta_az is -80 and we'd be going CCW to get there.

the .wrap_at('180d') also handles situations where you want to go from az of 10 to az of 350, the raw_delta_az will be -20 (due to the wrapping) and the direction will be CCW so delta_az will be positive.

fergusL and others added 5 commits October 11, 2019 15:34

threading example

95f8a6d

initial implementation of non-blocking moves

9186fe6

fixing up the calibration thread

82afd4e

non-blocking movement commands working/passing tests

f22dad1

fixing broken calibration thread condition

46ba494

fergusL requested review from wtgee and AnthonyHorton October 16, 2019 09:05

AnthonyHorton approved these changes Oct 16, 2019

View reviewed changes

wtgee approved these changes Oct 16, 2019

View reviewed changes

fergusL mentioned this pull request Oct 17, 2019

Make manual control trigger an unhomed state in Dome class #60

Open

Anthony and Wilfred review/suggested changes

62be24c

fergusL requested review from wtgee and AnthonyHorton October 17, 2019 09:04

fergusL marked this pull request as ready for review October 17, 2019 09:07

wtgee approved these changes Oct 17, 2019

View reviewed changes

tiny changes

df8a9dd

fergusL merged commit a74120e into AstroHuntsman:develop Oct 17, 2019

fergusL deleted the threading-#38-#42 branch October 17, 2019 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Threading #38 #42 #59

Threading #38 #42 #59

fergusL commented Oct 16, 2019

AnthonyHorton left a comment

AnthonyHorton Oct 16, 2019

fergusL Oct 17, 2019

AnthonyHorton Oct 16, 2019

fergusL Oct 17, 2019

AnthonyHorton Oct 16, 2019

fergusL Oct 17, 2019

fergusL Oct 17, 2019

AnthonyHorton Oct 16, 2019

fergusL Oct 17, 2019

wtgee left a comment

wtgee Oct 16, 2019

fergusL Oct 17, 2019

wtgee Oct 16, 2019

fergusL Oct 17, 2019

wtgee left a comment

wtgee Oct 17, 2019

fergusL Oct 17, 2019

fergusL Oct 17, 2019

		first_rotation = threading.Thread(target=self._simulate_rotation)
		first_rotation.start()

Threading #38 #42 #59

Threading #38 #42 #59

Conversation

fergusL commented Oct 16, 2019

AnthonyHorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtgee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtgee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment