Fix polar plugin #461

yatharthranjan · 2024-09-05T12:19:42Z

This fixes the auto-connect of polar devices
Searches for polar devices every 5 seconds until one is found
On connection to a new device, it stores the Polar device ID and always reconnects to it in the future. Only one ID is stored at once.
Pair a new device using the "Reset Polar Device ID" action button.
Tested with Polar Vantage V3
Also fixes reconnection of the device and reload functionality in the plugin
Closes Polar plugin uses hardcoded deviceId’s to establish the connection to the device #450, The ‘reload’ button functionality is not yet implemented for the Polar devices #453, There is no automatic reconnection after user has lost connection to a Polar device #454

this-Aditya

Hi @yatharthranjan, this PR looks good, but I have identified some areas for improvement in the code merged from PR #448 that could optimize the plugin for better performance and usability.

this-Aditya · 2024-09-05T14:08:26Z

plugins/radar-android-polar/src/main/java/org/radarbase/passive/polar/PolarManager.kt

+    private val ppIntervalTopic: DataCache<ObservationKey, PolarPpInterval> =
+        createCache("android_polar_pulse_to_pulse_interval", PolarPpInterval())
+    private val ppgTopic: DataCache<ObservationKey, PolarPpg> =
+        createCache("android_polar_ppg", PolarPpg())

    private val mHandler = SafeHandler.getInstance("Polar sensors", THREAD_PRIORITY_BACKGROUND)
    private var wakeLock: PowerManager.WakeLock? = null


Do we need a wakelock here? We are using it with the PARTIAL_WAKE_LOCK flag, which ensures that the CPU remains active while allowing the screen to turn off. We already have this functionality with RadarService and SourceService, which always run in the background. If we still need it, the wakelock should also be released after it has been acquired.

I have left it in for now as the app did lose connection with the device without this after a while of screen being off.

Okay, sorry I skipped it. I’ve recommented.

this-Aditya · 2024-09-05T14:11:00Z

plugins/radar-android-polar/src/main/java/org/radarbase/passive/polar/PolarManager.kt


    private val mHandler = SafeHandler.getInstance("Polar sensors", THREAD_PRIORITY_BACKGROUND)


Please include handler.close() in the onClose method.

this-Aditya · 2024-09-05T14:12:31Z

plugins/radar-android-polar/src/main/java/org/radarbase/passive/polar/PolarService.kt

    override val defaultState: PolarState
        get() = PolarState()


I do not see any use for PolarState, it could be removed and replaced with BaseSourceState instead.

I have left it for now as it does set hasAcceleration to true, we can remove it in future if not required (I don't think it matters that much TBH)

this-Aditya · 2024-09-05T14:24:29Z

plugins/radar-android-polar/src/main/java/org/radarbase/passive/polar/PolarService.kt

        manager as PolarManager
    }
+
+    fun savePolarDevice(deviceId: String) {
+        applicationContext.getSharedPreferences(SHARED_PREF_NAME, Context.MODE_PRIVATE)


We can declare sharedPreferences as a variable to avoid repeatedly calling applicationContext.getSharedPreferences.

Suggested change

applicationContext.getSharedPreferences(SHARED_PREF_NAME, Context.MODE_PRIVATE)

class PolarService : SourceService<BaseSourceState>() {

private lateinit var sharedPreferences: SharedPreferences

override fun onCreate() {

super.onCreate()

// Instantiating sharedPreferences inside onCreate to make sure that service context is available

sharedPreferences = getSharedPreferences(SHARED_PREF_NAME, Context.MODE_PRIVATE)

}

}

this-Aditya · 2024-09-05T14:27:16Z

plugins/radar-android-polar/src/main/java/org/radarbase/passive/polar/PolarService.kt

        handler = SafeHandler.getInstance("Polar", Process.THREAD_PRIORITY_FOREGROUND)
    }


The handler instance is not being utilized here, we could remove it.

this-Aditya · 2024-09-05T14:44:27Z

plugins/radar-android-polar/src/main/java/org/radarbase/passive/polar/PolarManager.kt

                status = SourceStatusListener.Status.DISCONNECTED // red circle



Directly setting the source status to DISCONNECTED could disrupt the logic in AbstractSourceManager and SourceService. Therefore, none of the plugins set the DISCONNECTED status directly. Instead, we might consider using AbstractSourceManager::disconnect. However, this approach could introduce issues, potentially causing a continuous loop of connecting and disconnecting the plugin.

The connection and disconnection logic can be inspired by the implementations in Empatica and Faros. For example, they override superclass methods to fit their needs, like here in E4Service.

The exact implementation logic needs to be tested on the device to ensure its effectiveness.

Yes this is something that needs to be worked on and tested. There are already issues related to it #453 and #454. I am going to leave this for separate PR in the future. But please feel free to add a new issue to track this.

this-Aditya · 2024-09-05T15:01:03Z

plugins/radar-android-polar/src/main/java/org/radarbase/passive/polar/PolarManager.kt

+        autoConnectDisposable = Flowable.interval(0, 5, TimeUnit.SECONDS)
+            .observeOn(AndroidSchedulers.mainThread())


I am not very familiar with RxJava, but we are using observeOn(AndroidSchedulers.mainThread()) in every method. It seems that this forces the code inside these methods to always execute on the main thread, which could potentially freeze the UI

Yes, that is a good spot; I am also not very familiar with RxJava but this was provided in Polar documentation, hence being used here. I will try to remove it and test it. However, at least here it does not freeze the UI, as it only runs initially until the device is paired. Once there is a paired device, it will always connect to that and will not need scanning of devices. But I agree that sending the data could be an issue.

Alternatively, we can use observerOn with the main thread and simply wrap the code inside the methods using a handler if there are no UI updates.

this-Aditya · 2024-09-05T15:01:46Z

plugins/radar-android-polar/src/main/java/org/radarbase/passive/polar/PolarManager.kt

+                send(
+                    batteryLevelTopic,
+                    PolarBatteryLevel(name, currentTime, currentTime, batteryLevel)
+                )


Safehandler instance should be used to send data in background.

yatharthranjan · 2024-09-06T13:11:39Z

Hi @this-Aditya, thanks for the comments. I also recognised various issues with the existing polar impl. but only made changes in this PR to fix the auto-connect, as there are other issues in this repo for those things that can be handled in specific PRs (and also wanted to test this in isolation). However, I will incorporate your suggestions (and delegate to separate PR/issue where suitable). Thanks

this-Aditya · 2024-09-06T15:11:28Z

Oh, okay. I thought this was the finalized PR for the Polar plugin, which is why I mentioned these here 😅

this-Aditya

LGTM, thanks.

yatharthranjan · 2024-09-09T12:19:30Z

Oh, okay. I thought this was the finalized PR for the Polar plugin, which is why I mentioned these here 😅

No worries at all, very valuable feedback. I have tried to incorporate it where possible and added some other changes. This now closes #450, #454, #453.

this-Aditya

Thanks @yatharthranjan for updating this PR. It looks great so far. Could you please review these additional comments and include them if necessary?

Thanks

this-Aditya · 2024-09-09T13:18:00Z

plugins/radar-android-polar/src/main/java/org/radarbase/passive/polar/PolarManager.kt

@@ -125,8 +123,7 @@ class PolarManager(
            override fun deviceDisconnected(polarDeviceInfo: PolarDeviceInfo) {
                logger.debug("Device disconnected ${polarDeviceInfo.deviceId}")
                isDeviceConnected = false
-                status = SourceStatusListener.Status.DISCONNECTED // red circle
-
+                disconnect()


I am not entirely sure, but does it not loop between connecting and disconnecting the manager when this callback is received? If so, we could add logic specific to the needs of this plugin ( such as handling the merging of ::disconnect with the UNAVAILABLE status or similar to this) to prevent the loop and starting from READY again during reconnections.

Please disregard this review if the behavior is functioning correctly with ::disconnect as well.

I don't think I completely understand; when the device is disconnected, this callback is called, which sets the plugin state to disconnect. The recording is started again, and it becomes ready, looking for the device to connect again.

Here are logs showing the process

2024-09-09 16:01:27.858 19539-19539 pRMT:PolarManager org.radarcns.detail D Device disconnected CF27042A 2024-09-09 16:01:27.859 19539-19539 pRMT:SourceServiceConn* org.radarcns.detail I AppSource status changed of source Polar Vantage V3 CF27042A 2024-09-09 16:01:27.859 19539-19539 pRMT:SourceServiceConn* org.radarcns.detail I Updated source status to DISCONNECTING 2024-09-09 16:01:27.859 19539-19539 pRMT:RadarService org.radarcns.detail I Source of ServiceConnection<org.radarbase.passive.polar.PolarService> was updated to DISCONNECTING 2024-09-09 16:01:27.874 19539-19539 pRMT:SourceServiceConn* org.radarcns.detail I AppSource status changed of source null 2024-09-09 16:01:27.874 19539-19539 pRMT:SourceServiceConn* org.radarcns.detail I Updated source status to DISCONNECTED 2024-09-09 16:01:27.874 19539-19539 pRMT:RadarService org.radarcns.detail I Source of ServiceConnection<org.radarbase.passive.polar.PolarService> was updated to DISCONNECTED 2024-09-09 16:01:27.875 19539-19665 pRMT:RadarService org.radarcns.detail I Starting recording on connection ServiceConnection<org.radarbase.passive.polar.PolarService> 2024-09-09 16:01:27.880 19539-11102 pRMT:SourceService org.radarcns.detail I Starting recording now for PolarService 2024-09-09 16:01:27.883 19539-11102 pRMT:PolarManager org.radarcns.detail D Connecting to Polar API 2024-09-09 16:01:27.883 19539-19539 pRMT:SourceServiceConn* org.radarcns.detail I AppSource status changed of source Polar 2024-09-09 16:01:27.883 19539-19539 pRMT:SourceServiceConn* org.radarcns.detail I Updated source status to READY 2024-09-09 16:01:27.884 19539-19539 pRMT:RadarService org.radarcns.detail I Source of ServiceConnection<org.radarbase.passive.polar.PolarService> was updated to READY

I did not notice any looping in my testing.

Okay, that’s great. I have observed this looping behavior in other plugins, but I have not worked with any wearable devices for pRMT, so I was not sure about the behavior for wearables on ::disconnect

Thanks for the logs.

There is new work after this pr is accepted

afolarin · 2024-09-09T21:29:24Z

Nice work 👍🏽 @this-Aditya @yatharthranjan briefed me on some of the caveats, but this will still definitely be very useful, particularly as we push for good gold standards for benchmarking studies.

this-Aditya · 2024-09-10T04:50:09Z

Thanks @afolarin, glad to hear it.

yatharthranjan added 2 commits August 20, 2024 10:49

Merge remote-tracking branch 'origin/dev' into audio-input

b172538

Auto connect with Polar devices

2ccb1d8

yatharthranjan requested review from afolarin and this-Aditya September 5, 2024 12:19

this-Aditya mentioned this pull request Sep 5, 2024

Added New Plugin For Recording Audio #449

Merged

this-Aditya and others added 4 commits September 5, 2024 18:22

Merge branch 'dev' into fix-polar

4d06af4

Minor changes

cb1928d

Utilizing appContext from service instance

d19cfa9

Replacing android.util.Log with slf4j logger

47013d4

this-Aditya reviewed Sep 5, 2024

View reviewed changes

this-Aditya previously approved these changes Sep 9, 2024

View reviewed changes

PR feedback and other improvements

6c9c0e6

yatharthranjan mentioned this pull request Sep 9, 2024

Add Polar-autoscan plugin #451

Closed

yatharthranjan changed the title ~~Fix polar auto-connect~~ Fix polar plugin Sep 9, 2024

this-Aditya reviewed Sep 9, 2024

View reviewed changes

this-Aditya approved these changes Sep 9, 2024

View reviewed changes

yatharthranjan merged commit 02744c8 into dev Sep 9, 2024
2 of 3 checks passed

yatharthranjan deleted the fix-polar branch September 9, 2024 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix polar plugin #461

Fix polar plugin #461

yatharthranjan commented Sep 5, 2024 •

edited

Loading

this-Aditya left a comment

this-Aditya Sep 5, 2024 •

edited

Loading

yatharthranjan Sep 9, 2024 •

edited

Loading

this-Aditya Sep 9, 2024

this-Aditya Sep 5, 2024

this-Aditya Sep 5, 2024

yatharthranjan Sep 6, 2024

this-Aditya Sep 5, 2024 •

edited

Loading

this-Aditya Sep 5, 2024

this-Aditya Sep 5, 2024 •

edited

Loading

yatharthranjan Sep 6, 2024

this-Aditya Sep 5, 2024 •

edited

Loading

yatharthranjan Sep 6, 2024

this-Aditya Sep 6, 2024 •

edited

Loading

this-Aditya Sep 5, 2024 •

edited

Loading

yatharthranjan commented Sep 6, 2024

this-Aditya commented Sep 6, 2024

this-Aditya left a comment

yatharthranjan commented Sep 9, 2024

this-Aditya left a comment

this-Aditya Sep 9, 2024

yatharthranjan Sep 9, 2024

this-Aditya Sep 9, 2024

afolarin commented Sep 9, 2024

this-Aditya commented Sep 10, 2024


		private val mHandler = SafeHandler.getInstance("Polar sensors", THREAD_PRIORITY_BACKGROUND)

-        applicationContext.getSharedPreferences(SHARED_PREF_NAME, Context.MODE_PRIVATE)
+class PolarService : SourceService<BaseSourceState>() {
+    private lateinit var sharedPreferences: SharedPreferences
+    override fun onCreate() {
+        super.onCreate()
+        // Instantiating sharedPreferences inside onCreate to make sure that service context is available
+        sharedPreferences = getSharedPreferences(SHARED_PREF_NAME, Context.MODE_PRIVATE)
+    }
+}

		handler = SafeHandler.getInstance("Polar", Process.THREAD_PRIORITY_FOREGROUND)
		}

		status = SourceStatusListener.Status.DISCONNECTED // red circle

		autoConnectDisposable = Flowable.interval(0, 5, TimeUnit.SECONDS)
		.observeOn(AndroidSchedulers.mainThread())

Fix polar plugin #461

Fix polar plugin #461

Conversation

yatharthranjan commented Sep 5, 2024 • edited Loading

this-Aditya left a comment

Choose a reason for hiding this comment

this-Aditya Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

yatharthranjan Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

this-Aditya Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

this-Aditya Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

this-Aditya Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

this-Aditya Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

this-Aditya Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

yatharthranjan commented Sep 6, 2024

this-Aditya commented Sep 6, 2024

this-Aditya left a comment

Choose a reason for hiding this comment

yatharthranjan commented Sep 9, 2024

this-Aditya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afolarin commented Sep 9, 2024

this-Aditya commented Sep 10, 2024

yatharthranjan commented Sep 5, 2024 •

edited

Loading

this-Aditya Sep 5, 2024 •

edited

Loading

yatharthranjan Sep 9, 2024 •

edited

Loading

this-Aditya Sep 5, 2024 •

edited

Loading

this-Aditya Sep 5, 2024 •

edited

Loading

this-Aditya Sep 5, 2024 •

edited

Loading

this-Aditya Sep 6, 2024 •

edited

Loading

this-Aditya Sep 5, 2024 •

edited

Loading