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

Support Android 14 #413

Conversation

PavlosTze
Copy link
Contributor

Fixes Issue 412

@CLAassistant
Copy link

CLAassistant commented Nov 3, 2023

CLA assistant check
All committers have signed the CLA.

@sylwester-zielinski sylwester-zielinski merged commit 81cc49e into NordicSemiconductor:main Nov 3, 2023
1 check passed
@philips77
Copy link
Member

I don't think the fix is correct. The action Intents are sent using LocalBroadcastManager, an ancient API to make sending local broadcasts faster, as it doesn't require using the OS for that purpose. The received, as far as I know, is never registered in the OS, therefore should not be a subject for change regarding Android 14. I'll have to double check it by looking into the sources.

The proposed fix changes behavior for Android 14+. Now, the intents must be sent using the OS and the "local" ones are ignored.

Are you sure the crash in #412 was from this manager.registerReceiver... call?

this.registerReceiver(mDfuActionReceiver, actionFilter, RECEIVER_NOT_EXPORTED);
} else {
manager.registerReceiver(mDfuActionReceiver, actionFilter);
}
// Additionally we must register this receiver as a non-local to get broadcasts from the notification actions
ContextCompat.registerReceiver(this, mDfuActionReceiver, actionFilter, ContextCompat.RECEIVER_NOT_EXPORTED);
Copy link
Member

Choose a reason for hiding this comment

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

Now the OS receiver is registered twice, as this line does the same as 1039. We're using ContextCompat here to avoid the need for branching based on version number.

@PavlosTze
Copy link
Contributor Author

PavlosTze commented Nov 5, 2023

@philips77 LocalBroadcastManager is ancient, as you said, therefore doesn't have the RECEIVER_EXPORTED or RECEIVER_NOT_EXPORTED property. Also yes I'm 100% sure the crash occured because of that, as we tested 2.3.2 version already and fixed that crash.

Another crash occurred though, in a different line, because of another change in Android 14 that hasn't been managed properly on this library.

New issue here: #414

@PavlosTze PavlosTze deleted the feature/support-android-14 branch November 5, 2023 22:07
@philips77
Copy link
Member

Also yes I'm 100% sure the crash occured because of that, as we tested 2.3.2 version already and fixed that crash.

Well... perhaps, but it also ignores all broadcasts sent using LocalBroadcastReceiver.

MiriamTom pushed a commit to MiriamTom/Gadgetbridge that referenced this pull request Oct 5, 2024
Since we raised `targetSdkVersion` to 34 in fad092b,
attempt to upgrade Pinetime firmware on Android 14+ causes Gadgetbridge
to die with "One of RECEIVER_EXPORTED or RECEIVER_NOT_EXPORTED should be
specified when a receiver isn't being registered exclusively for system
broadcasts" unhandled java.lang.SecurityException.

This was caused by the Nordic Semi DFU library not supporting A14 until
version 2.3.2 (see NordicSemiconductor/Android-DFU-Library#412
and NordicSemiconductor/Android-DFU-Library#413).

So, upgraded the library to the current release 2.5.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants