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

Fixed issue with incorrect temporary target validation #4861

Merged
merged 2 commits into from
Aug 11, 2019

Conversation

Erin879
Copy link
Contributor

@Erin879 Erin879 commented Aug 6, 2019

This fixes the issue described in #4859, by only running the validation code if the temporary target event is actually a target rather than a cancel event.

@Erin879
Copy link
Contributor Author

Erin879 commented Aug 7, 2019

Perhaps a better solution would be to remove the code here which turns Temporary Target Cancel events into Temporary Target events, and then that'd avoid the issue entirely. However, I'm unsure on why this code was implemented so I'd rather not include that change in a commit without other people's input.

EDIT: I spent some time reading over the code to try and find any issues with this approach, and the only one I could think of was that previously stored Temporary Target Cancel events would be treated as Temporary Target events. Maybe this could be fixed by removing the code to save cancel events as normal temp targets, and then having two methods (duration of 0, and cancel events) to parse the events, and eventually removing the legacy code? However, this might cause issues with how openaps handles cancelling temp targets, but as I use androidaps I am unsure on that.

@sulkaharo
Copy link
Member

The reason we set a new target with 0 duration is simply that the way the data is interpreted is, a new target automatically overrides the previous, and thus a 0 length target cancels the previous target without adding a new target. This is how the data is also interpreted by systems that read Nightscout, so there's no easy way to provide backward compatibility if we add a new event type

@sulkaharo sulkaharo merged commit 0f7c03d into nightscout:dev Aug 11, 2019
@mhaeberli
Copy link

When can we expect this to be merged back into master?

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.

3 participants