-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
G7 battery warning #3239
G7 battery warning #3239
Conversation
@Navid200: Stupid question - how relevant is this for G7? Doesn't G7 only run MAX 10 days + 12 hours and then it shuts down? How come your G7 battery graph screenshots are going up to 35 days - is that a G6 or a somehow modified G7? Thanks for explaining. |
@emp-00 You can still establish connectivity with a G7 even after 10.5 days to see what the battery voltages are like. The only reason I am monitoring these voltages after 10 days is to see at what point the battery really dies. As far as this PR is concerned, I am showing the delta between Va and Vb. The delta is no where near 10. That's the only thing I would like to demonstrate. |
@Navid200 : That's where I was coming from - G7 stops working (by design) after 10.5 days, so why would anybody in "real diabetic life" keep it running longer? If you don't receive readings any more you start another sensor and pull out the old one. As long as the 10.5 days cannot be extended I don't think much work needs to be put into battery life and warning levels. Just my thought on this. |
@emp-00 You can receive a G7 and put it in storage for 5 years. If you then start using it, it is very likely that it will not run for 10.5 days. And in that case, I expect voltage B to show very low and I expect it to be red then. So, there is no guarantee that a G7 will run for 10.5 days and there is a value in having valid warning levels in xDrip. |
Understood, all good :-) |
Hi Navid, so I support the change for the voltageb warning but please use the I think its very risky to mess around with the setting of the g6 defaults logic because this was really somewhat of a workaround for people transitioning from g5 to g6 or who otherwise have broken settings. I don't really want to integrate those changes. I personally don't think we should support custom battery warning levels on g7 as we expect them to be uniform enough that if a new revision needs changes to the defaults we would want to know about that rather than people needing to update the setting themselves. I think we can look to see if they have already customized the value and leave it alone if so to support people who might move back and forwards between g6 and g7 but in general I don't think we need to provide the same support for battery level management because it shouldn't require customization for g7 and really we just need to inform the user if their g7 battery looks duff vs what is generally seen in the wild. |
@jamorham Understood. Instead of making short tx id public, shouldn't we create an option for the user to choose G7? But, if we allow the user to choose G7, when the new device comes out, G7 still will be the only device working and we can only add the new device perhaps with another option in the wizard for the user to choose. Can I add that option instead of making short txid public? |
Will be tested before tomorrow. |
I have made the change you asked for and tested. |
@@ -197,4 +197,4 @@ public static String getLastTwoCharacters(final String txid) { | |||
if (txid == null) return "NULL"; | |||
return txid.length() > 3 ? txid.substring(txid.length() - 2) : "ERR-" + txid; | |||
} | |||
} | |||
} |
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.
add new line
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.
Done
@@ -93,6 +95,9 @@ public boolean voltageAWarning() { | |||
} | |||
|
|||
public boolean voltageBWarning() { | |||
if (shortTxId()) { // G7 only | |||
return voltageB() < (G5BaseService.LOW_BATTERY_WARNING_LEVEL - 25); |
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.
it would be best if 25 is a constant
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.
This is a copy of a code that uses 10, which was not a constant. I just followed that practice.
Thanks |
This PR makes 2 changes.
1- If you currently customize the battery warning level for G7, after the next read cycle is completed, it will automatically be rest to 280. Someone reported this on facebook. I was able to recreate the problem.
This PR changes that so that you can customize the warning level and it will not be reset.
Fixes: #3205
2- The voltage B is highlighted red for almost everyone on the status page for G7.
This PR changes how the voltageB warning level is extracted.
The delta of 10 may have been valid, between voltage A and voltage B, for G5. But, it has not been valid for G6 and it is not valid for G7.
I am still collecting data. So, this is preliminary:
Instead of subtracting 10 from the voltage A warning level to come up with the voltage B warning level, I am subtracting 25.
We may actually need to even go to 30. But, for now, this should be OK.
This PR makes no change to G5 or G6.
Tests:
I have tested this with G7 and G6.