ImperialCollegeLondon / gridlington-vis

BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

Assign functionality to start & stop buttons #109

Closed tsmbland closed 10 months ago

tsmbland commented 10 months ago

Description

This PR adds functionality to the start, stop and restart buttons in the control app.

Most of the changes can be found in control.py. This used to contain one large callback function (update_button_click) responsible for all the button callbacks (which were mostly empty), but since #92 Adrian has started moving button callbacks into their own functions which I think is a good move so I've followed suit here. I have written three new callbacks for the start, stop and restart buttons. The start and stop buttons are responsible for starting/stopping the live model, which it does via the new start_model and stop_model functions in datahub_api.py. These interact with the DataHub API introduced in this PR, which modifies the DataHub variable model_running which is tracked by the model. I hope I've done this correctly, however it's difficult for me to test without access to the live model. The start/stop buttons also interact with data_interval to start/stop data and figure updates in the vis system. The restart button sends data_interval back to the beginning, which resets the pre-set data. It already effectively did this when connected to the OVE (via core.refresh_sections()), but now it also works outside the OVE. In theory I think this button should also send a restart signal to the model via the DataHub, but it doesn't seem to me like this is possible yet, so I've left this as a TODO. (EDIT: Actually I think stopping the model also restarts it, so this would just be a stop call followed by a start call - I will update)

I also had to rewrite some of the tests. The diffs look messy, but really I'm just moving the tests for each button into their own functions, and modifying tests for the start, stop and restart buttons in accordance with my changes.

Close #41 #61 #48

Type of change

Key checklist

tsmbland commented 10 months ago

Having said that, I am not entirely sure this will have any effect in the backend. The functions you have added connect to endpoints that, essentially, just set flags. But those flags, as far as I can tell, do nothing. They are not used anywhere else to define the behavior of the model. For example, the reset_data function in here, which presumably brings the data model back to its initial state, is not used.

As discussed, I believe Yue's model will check the DataHub for the model_running flag to decide when to start/stop the model (see here and here). When stopping the model, I belive the model will then use the signal_model_ready API call to indicate to the DataHub when the model is ready to start again by setting the model_resetting flag. We probably need to use this flag in either the vis system or the DataHub to prevent the model from starting until it's ready, but this functionality doesn't appear to be in place without changes to the DataHub (as far as I can tell). Also, you're right that the reset_data function isn't used at all, and I imagine this should be included in one of the API calls.

I'm going to leave everything as it is for now and merge it, and will suggest that Adrian takes a look at this PR when he's back. I think it won't need any drastic changes, but Adrian knows how all the parts interact better than I do.

tsmbland commented 8 months ago

@AdrianDAlessandro Can you check the start and stop buttons, and add the desired behaviour for the restart button? It all works when using the pre-set data, but not really sure what's required for the live model

AdrianDAlessandro commented 8 months ago

@AdrianDAlessandro Can you check the start and stop buttons, and add the desired behaviour for the restart button? It all works when using the pre-set data, but not really sure what's required for the live model

Yes I checked them yesterday, they make the expected changes in the datahub, but the model needs to be developed to hook into the datahub, which is not up to us.

tsmbland commented 8 months ago

Ok. The restart button doesn't send any signals yet, just refreshes the page and resets the counter. Is this what we want?

AdrianDAlessandro commented 8 months ago

Yea, should be fine. I can't see a simple way to send a stop and then start signal that doesn't cause a race condition with the Opal model