-
Notifications
You must be signed in to change notification settings - Fork 0
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
Assign functionality to start & stop buttons #109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have comments, not anything to change.
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.
In summary, this PR looks good and is needed, but I'm not sure if it will work as intended without appropriate changes in the datahub.
As discussed, I believe Yue's model will check the DataHub for the 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. |
@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. |
Ok. The restart button doesn't send any signals yet, just refreshes the page and resets the counter. Is this what we want? |
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 |
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 newstart_model
andstop_model
functions indatahub_api.py
. These interact with the DataHub API introduced in this PR, which modifies the DataHub variablemodel_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 withdata_interval
to start/stop data and figure updates in the vis system. The restart button sendsdata_interval
back to the beginning, which resets the pre-set data. It already effectively did this when connected to the OVE (viacore.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
python -m pytest
)pre-commit run --all-files
)