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

[ExposureNotification] Implement the shared API for v2 #955

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

mattleibow
Copy link
Collaborator

@mattleibow mattleibow commented Aug 26, 2020

Generally, the two platforms are similar, but there are some differences.

The biggest difference I think is that iOS "extended" their ENExposureConfiguration in an interesting way. They just moved all the previous member to the bottom and then added all the new ones at the top. And then added a flag to say which one.

Android was a bit better and created a "separate" API with new configuration types. This is better in the fact that there is no overlap. And, they also split operations up again, so there are two new configurations. So, now iOS still has one big thing and Android has 3: 1 obsolete, 2 smaller.

I have created a "v2" handler that returns a new configuration object. So, instead of implementing IExposureNotificationHandler, you can implement the IExposureNotificationDailySummaryHandler to support the "Exposure Window" mode. This is an additive feature to support older devices:

public interface IExposureNotificationDailySummaryHandler : IExposureNotificationHandler
{
	Task<DailySummaryConfiguration> GetDailySummaryConfigurationAsync();
	Task ExposureStateUpdatedAsync(IEnumerable<ExposureWindow> windows, IEnumerable<DailySummary>? summaries);
}

The app will just have to add this additional interface to the existing handler implementation. This is an additive handler because it could exist that a device might be running an older version of Google Play Services that does not yet support the v1.5 and v1.6 APIs. Or if the iOS device is not yet updated.

In order for the library to use the new APIs, there are 2 requirements:

  1. The handler implements IExposureNotificationDailySummaryHandler
  2. The device supports the new APIs

@JanettHolst290490
Copy link

Could there be a way to define if we want to use Android v1.5 or v1.6, since it seems like v1.6 will require Android 11, which we might not get ready for.
Or will it require v1.6 and Android 11 already? I suppose we will be able to support both Android 11 and older versions still. But it would be nice if we could wait with Android 11 support.

I suppose iOS 14 will be a requirement.

@mattleibow
Copy link
Collaborator Author

@JanettHolst290490 sorry for the long delay, we are hoping to get this done very soon. Based on my work so far, I think we are very close, I will just need to see how we can test because I don't have all the required access to the API because neither I nor MS are "Authorized Health Providers". However, this is not too much of a big deal as we think we have most of the things ready and then your feedback will help us fix up any issues.

So, to address your questions:

Could there be a way to define if we want to use Android v1.5 or v1.6, since it seems like v1.6 will require Android 11, which we might not get ready for. Or will it require v1.6 and Android 11 already?

This is automatically handled. The APIs are actually bound to Google Play Services, not the version of Android. This means that this code should work on any version that has the various versions of the services. In addition to that, we actually check each of the available APIS, so it basically tries v1.6, and if that is not there, then it tries v1.5 and finally it falls back to the v1.0.

I suppose iOS 14 will be a requirement?

Similar to Android, it will fallback gracefully since we detect the version of the OS before trying to use new APIs.

Both of these questions are why we decided to go with the new handler as a derived handler. This basically means that it can fall back if necessary.

public interface IExposureNotificationDailySummaryHandler : IExposureNotificationHandler
{
	Task<DailySummaryConfiguration> GetDailySummaryConfigurationAsync();

	Task ExposureStateUpdatedAsync(IEnumerable<ExposureWindow> windows, IEnumerable<DailySummary>? summaries);
}

@mattleibow mattleibow marked this pull request as ready for review November 6, 2020 22:21
@mattleibow
Copy link
Collaborator Author

I have also started updating the sample app based on these new APIs. It is not yet complete, but it should have a starting point: xamarin/ExposureNotification.Sample#86

@pavshr
Copy link

pavshr commented Nov 9, 2020

Hello @mattleibow ,

According to this, there is an important improvement in 1.5 SDK for Android, namely:

App force-stop improvements: if an app has been force-stopped in the background, the Exposure Notifications service will wake it up again automatically.

As I see in the log for this PR, this cross-platform lib is planned to be build on Android 1.7.1 SDK, does it mean that we will also get this "force-stop improvements" when updating to the new nuget-package for shared EN API? Or some extra actions will be required?

@mattleibow
Copy link
Collaborator Author

@pavshr according to the release notes, it looks like that feature was implemented in 1.5, and this PR builds with 1.6. This should mean that these improvements should come automatically.

@nc-kano
Copy link

nc-kano commented Dec 23, 2020

@mattleibow
Copy link
Collaborator Author

@nc-kano There is an issue for that here: #1066
I'm just typing out some info.

w.ScanInstances.Select(s => s.FromNative())));
}

internal static async Task PlatformUpdateDiagnosisKeysDataMappingAsync()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find the code calling this method in this PR...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you search PlatformGetExposureWindowsAsync ?

internal static async Task<IEnumerable> PlatformGetExposureWindowsAsync()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry. I might have confused you.
Where is PlatformUpdateDiagnosisKeysDataMappingAsync called?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, @tmurakami, this is not called in the nuget code. We ended up into calling SetDiagnosisKeysDataMappingAsync manually before calling UpdateKeysFromServer. Ideally, this should be called on Android after getting keys and configuration, so it won't send one more request to the server to fetch configuration one more time. There is, apparently, a limit on number of time how often this API can be called, so I would recommend either catching the exception when this limit is reached, or ensure that the method is not called more often than once a week (src)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavshr Thank you for sharing the information ❤️

@nc-kano
Copy link

nc-kano commented Feb 24, 2021

As i have noted in #1097 EN v2 features should be marked as introduced in iOS 12.5

Comment on lines +358 to +363
if (IsDailySummaries)
{
// Check that the summary has actual data before notifying the callback
if (detectionSummary.DaySummaries.Length > 0)
{
var (windows, dailySummary) = await PlatformProcessSummaryV2Async(m, detectionSummary, cancellationToken);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, I am actively working on testing the Nuget package with these changes. Based on what I see on iOS, await DailySummaryHandler!.ExposureStateUpdatedAsync(windows, dailySummary) is never called, indicating that something here might be out of control. ExposureDetectedAsync is not called either. I managed to apply the new configuration object, and in the phone settings I see that the analysis of the submitted keys with the new configuration went well and multiple matches are found, however, ExposureStateUpdatedAsync() on the handler is never called.

No exceptions are thrown.

image

@mattleibow

@tmurakami Did you test it out as well? Do you have the same issue?
Thanks in advance!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavshr I'm not sure because I can't test it, but I guess you need to assign Infectiousness to all the keys (-14 to 14).
https://github.com/folkehelseinstituttet/Fhi.Smittestopp.App/blob/044831fe92695866643d5c14517aaaf2a00002c3/NDB.Covid19/NDB.Covid19/ExposureNotifications/ExposureNotificationHandler.cs#L168-L173

dsc.DaysSinceOnsetInfectiousness = new Dictionary<int, Infectiousness>()
{
    [-14] = Infectiousness.Standard, // Standard, High, or None
    [-13] = Infectiousness.Standard,
    [-12] = Infectiousness.Standard,
    *snip*
    [-1] = Infectiousness.Standard,
    [0] = Infectiousness.Standard,
    [1] = Infectiousness.Standard,
    *snip*
    [12] = Infectiousness.Standard,
    [13] = Infectiousness.Standard,
    [14] = Infectiousness.Standard,
};

See Configure Criteria to Estimate Risk for more details.
https://developer.apple.com/documentation/exposurenotification/building_an_app_to_notify_users_of_covid-19_exposure

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattleibow The same results from my debugging. It seems detectionSummary.DaySummaries.Length is always 0 or even null.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using the V2 TEK format in the zip file (with DSOS and ReportType), iOS' ExposureStateUpdatedAsync is called. So there is no issue here, just managed to verify it yesterday.

FYI @tmurakami

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ns-kano @pavshr
I had the same problem. I found out we can set the parameter infectiousnessWhenDaysSinceOnsetMissing to infectiousnessForDaysSinceOnsetOfSymptoms.

keiji/chino@f6032cc#diff-1d4d67341f260be73d3e12f741daf36c4e17020b81c566ca3a56cfc10fb0ca59R383-R391

You can see the code in Server.swift, a reference app provided by Apple.

I didn't try to V2 TEK format. I am using TEK Export Tool from exposure-notifications-server.


Task<IEnumerable<ExposureInfo>> GetInfo()
// we used windows mode and implemented the windows mode handler
if (token == ExposureNotificationClient.TokenA && ExposureNotification.DailySummaryHandler != null)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is been a while since this PR was made, and Google apparently went out from Token concept (link, link2).
Now, they are recommending to use token-less version of the API. Would you consider changing this?
image

@pavshr
Copy link

pavshr commented Apr 8, 2021

There is an interesting difference in how MaximumScore, ScoreSum and WeightedDurationSum are seen in the app based on the platform:

iOS

image

Android

image

The pictures you see are from two phones being exposed to each other and reported infected.

@nc-kano
Copy link

nc-kano commented Apr 12, 2021

@mattleibow During investigation of the scores on the iOS we have found that weights for iOS are set incorrectly.
On iOS weights should be a number from 0 to 250
On Android weight should be number from 0.0 to 2.5
so weights on iOS needs to be multiplied by 100
That is why iOS is returning 60.0 currently
FYI @tmurakami

@nc-kano
Copy link

nc-kano commented Apr 27, 2021

@mattleibow can we have a updated version of Xamarin.GooglePlayServices.Nearby.ExposureNotification?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants