openaps / oref0

oref0: The open reference implementation of the OpenAPS reference design.
http://www.OpenAPS.org
MIT License
431 stars 395 forks source link

preferences.json contains all defaults #590

Closed thebookins closed 7 years ago

thebookins commented 7 years ago

This code is designed to expose only some of the configurable settings to the user through preferences.json. However, on a new setup from dev all default settings are in preferences.json (e.g. new or rarely touched settings like enableSMB_after_carbs and remainingCarbsCap). Having all of these settings in preferences could be confusing for users unaware of their effects. According to @scottleibrand the issue is at https://github.com/openaps/oref0/blob/dev/bin/oref0-setup.sh#L575.

Bender1061 commented 7 years ago

my workaround for this issue is I use WinSCP, I move the Preferences.json to my computer first, then after the Putty shows the preferences being made during the install, I then just copy it back to the rig. Right now we would need to made a script to check to make sure all the preferences are in the preferences.json, as it has grown by a lot since we first started. so we can't just see if the file is there and not overwrite.

tim2000s commented 7 years ago

Easiest thing to do is include a line in the setup script that asks if we want to save existing preferences and if so copy it to a safe place then copy per intent parts back.

Bender1061 commented 7 years ago

That would probably work, but what about the people that are currently running the same stuff from before oref1?

scottleibrand commented 7 years ago

Anyone care to test https://github.com/openaps/oref0/pull/609 and see if it gives the expected behavior?

PieterGit commented 7 years ago

I think I'm -1 on this change, because it violates "Explicit is better than implicit", https://www.python.org/dev/peps/pep-0020/

What's the point of hiding defaults to users?

scottleibrand commented 7 years ago

Because users are always asking "what should I set this to" and wanting to change things they shouldn't be changing. This is a way to allow for things that we'd otherwise hard-code to be configurable, without making users thing they should configure them.

scottleibrand commented 7 years ago

(Most of the rest of Zen of Python is trumped by "practicality beats purity" IMO.)

PieterGit commented 7 years ago

The example of enableSMB_after_carbs is a feature that I think you can ask users to enable or disable. The SMB-docs suggest: "You may want to experiment with turning only one of them on at a time so you can closely observe the behavior", http://openaps.readthedocs.io/en/master/docs/Customize-Iterate/oref1.html?highlight=turning#how-to-turn-on-smb-uam

If we want immutable preferences, we should not call them prefences but constants and implement them as something else, just like a Java static const (don't know the Javascript name for it).

Still -1 on it. Maybe for 0.6.x it would be good to separate:

It's a pity json doesn't support comments, so that the configuration/preference can be more verbose.

scottleibrand commented 7 years ago

We're not talking about immutable preferences. We're talking about things that certain advanced users want to change (like maxCOB, override_high_target_with_low, skip_neutral_temps, bolussnooze_dia_divisor, min_5m_carbimpact, carbratio_adjustmentratio, autotune_isf_adjustmentFraction, remainingCarbsFraction, and remainingCarbsCap) but that regular users shouldn't have to read up on and try to understand just to get started. Anyone can add any of these to their preferences.json manually: this setting just controls which ones get put in by default on initial install.

There are certain other preferences like the enableSMB, and newer features like unsuspend_if_no_temp and the exponential curve ones, that we currently don't want exposed to newbies, but which we may sooner or later want to add to displayedDefaults.

PieterGit commented 7 years ago

@thebookins @scottleibrand : I think this has been fixed with https://github.com/openaps/oref0/commit/18b066d8b759d991cf3456b6659ff21b6f7c4ce4#diff-539af46e71a958ac792a8b10bab689b9 (and possibly other commits).

scottleibrand commented 7 years ago

Yes, this has been fixed, thanks.