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

Bugfix settings machine vars not persisting on subsequent data writes #1708

Conversation

avanwinkle
Copy link
Collaborator

@avanwinkle avanwinkle commented Aug 29, 2023

This PR fixes a bug that would occasionally (okay, often) forget saved machine vars and cause operator-configured settings to reset to default. After much digging and consternation, I believe I've figured out the cause and it's a simple fix.

The Background

When MPF initializes, it first reads all machine variables from the machine_vars.yaml file and then appends those with the (default) variables from the machine_variables: config section. All settings values are backed under-the-hood by machine variables, so this logic covers operator-adjustable settings as well.

The Bug

While the code flow for loading machine vars from config section includes the configuration of a persist value, loading machine vars from the data file does not, which means all saved settings are defaulted to persist: False. Which is okay for a short while, but eventually something else is going to write an update to the persisted data (a new setting adjustment, or a player score, or something).

The Impact

The way the logic of writing the data works is that the code looks through all the machine vars, finds all of the ones that are persist: True, and writes a complete new data file with those values. Remember from before that values previously loaded from the data file are persist: False, so all of those values get wiped.

The Fix

This PR tweaks the flow for loading persisted values from a data file, explicitly flagging them each as persist: True in the machine variables controller. After all, if the value was persisted before, it should be persisted still, right?

The updated persistence logic takes place after data validation and expiration checks, so it shouldn't impact data retention policies, and it fixes the issue of saved settings being forgotten on subsequent data writes.

machine vars be forgettin

@sonarcloud
Copy link

sonarcloud bot commented Aug 29, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@toomanybrians toomanybrians merged commit 80b3a9b into missionpinball:dev Aug 29, 2023
3 of 12 checks passed
@atummons
Copy link
Contributor

@avanwinkle Seeing that tests fail now. I looked at it a little bit, and it is probably just mock data doesn't matter in real life. But values that are expected to not persist are being set to true on the load with this change. I didn't want to just go fudge the data to pass the tests in case there is an edge case that is not being caught.

avanwinkle added a commit to avanwinkle/mpf that referenced this pull request Aug 30, 2023
avanwinkle added a commit that referenced this pull request Aug 31, 2023
…-persist

Fix tests from #1708 machine variable persistence
avanwinkle added a commit to avanwinkle/mpf that referenced this pull request Aug 31, 2023
…s-machine-vars-persist

Fix tests from missionpinball#1708 machine variable persistence
avanwinkle added a commit to avanwinkle/mpf that referenced this pull request Sep 5, 2023
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.

None yet

3 participants