Add property lastScanTime and lastRequestTime to WirelessDevice
ClosedPublic

Authored by meven on Aug 30 2019, 10:26 AM.

Details

Summary

To properly use the WirelessDevice::RequestScan, we need to listen to LastScan property change.

lastScanTime reflects the time when we receive a LastScan property change.
lastRequestScanTime reflects the time when we last requested a scan.

See: https://developer.gnome.org/NetworkManager/stable/gdbus-org.freedesktop.NetworkManager.Device.Wireless.html#gdbus-method-org-freedesktop-NetworkManager-Device-Wireless.RequestScan

Diff Detail

Repository
R282 NetworkManagerQt
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Aug 30 2019, 10:26 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 30 2019, 10:26 AM
Restricted Application added 1 blocking reviewer(s): jgrulich. · View Herald Transcript
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Aug 30 2019, 10:26 AM

I am hesitant to add a second field "previousScan" that would store the previous lastScan timeStamp when a new one arrives.
This would be used to compute the time elapsed between the scans which is necessary to do since Network manager requires scans to be at least 10 seconds apart :
https://github.com/NetworkManager/NetworkManager/blob/master/src/devices/wifi/nm-device-wifi.c#L1205

I am hesitant to add a second field "previousScan" that would store the previous lastScan timeStamp when a new one arrives.
This would be used to compute the time elapsed between the scans which is necessary to do since Network manager requires scans to be at least 10 seconds apart :
https://github.com/NetworkManager/NetworkManager/blob/master/src/devices/wifi/nm-device-wifi.c#L1205

That would be useful I guess and could be used in plasma-nm.

  1. You should set initial lastScan property to "-1" which should indicate there was no scan attempt
  2. Mention that it will work only when NM 1.12+ is running, older NM releases don't support this property
  3. Above mentioned will be a problem when you would like to rely on that in plasma-nm, because we should be working even with older NM versions
meven updated this revision to Diff 64983.Aug 30 2019, 11:57 AM

Change parameters

meven retitled this revision from Add property lastScan to WirelessDevice and associated change signal to Add property lastScanTime and lastRequestTime to WirelessDevice.Aug 30 2019, 11:57 AM
meven edited the summary of this revision. (Show Details)
meven edited the summary of this revision. (Show Details)
jgrulich added inline comments.Aug 30 2019, 12:12 PM
src/wirelessdevice.cpp
251

Don't use QDateTime::currentDateTime() as it might be different from the actual value returned by NetworkManager.

jgrulich added inline comments.Aug 30 2019, 12:15 PM
src/wirelessdevice.h
133

Add something like:
@note will always return invalid QDateTime when runtime NM < 1.12

229

Add something like:
@note will never be emitted when runtime NM < 1.12

I am hesitant to add a second field "previousScan" that would store the previous lastScan timeStamp when a new one arrives.
This would be used to compute the time elapsed between the scans which is necessary to do since Network manager requires scans to be at least 10 seconds apart :
https://github.com/NetworkManager/NetworkManager/blob/master/src/devices/wifi/nm-device-wifi.c#L1205

That would be useful I guess and could be used in plasma-nm.

  1. You should set initial lastScan property to "-1" which should indicate there was no scan attempt
  2. Mention that it will work only when NM 1.12+ is running, older NM releases don't support this property
  3. Above mentioned will be a problem when you would like to rely on that in plasma-nm, because we should be working even with older NM versions
  1. I missed your comment and made important code change, instead of using the LastScan property, I use QDateTime to retain requestScan event and LastScan changes event.
  1. will do
  1. My code in plasma should be compatible already D23578
src/wirelessdevice.cpp
251

LastScan is in CLOCK_BOOTIME which is complicated to work with.
So I use naively the signal time rather than the LastScan value.
This will need a bit of work just to work with it:
https://github.com/NetworkManager/NetworkManager/blob/a7d8fe0ea5eb7be42a86527226ea54fd221fb1b4/shared/nm-glib-aux/nm-time-utils.c#L193

meven updated this revision to Diff 64987.Aug 30 2019, 12:28 PM

Mention nm version limitation

meven marked 2 inline comments as done.Aug 30 2019, 12:28 PM
jgrulich added inline comments.Aug 30 2019, 12:39 PM
src/wirelessdevice.cpp
251

Then cannot you work with it as with qlonglong? It shouldn't matter then if it's in CLOCK_BOOTIME .

meven added inline comments.Aug 30 2019, 12:46 PM
src/wirelessdevice.cpp
251

I need to be able to know the elapsed time since LastScan. So it doesn't matter as long as I manage to get a CLOCK_BOOTIME value for current time.

Anyway I am thinking about adding a short util function somewhere to convert CLOCK_BOOTTIME to QDateTime.

meven updated this revision to Diff 65002.Aug 30 2019, 2:44 PM

Convert the LastScan value to a QDateTime, update function and doc in consequence

meven marked 4 inline comments as done.Aug 30 2019, 2:45 PM

I have implemented the CLOCK_BOOTTIME conversion to QDateTime in QDateTime NetworkManager::clockBootTimeToQDateTime

jgrulich added inline comments.Sep 2 2019, 8:25 AM
src/utils.h
86
  1. Missing NETWORKMANAGERQT_EXPORT
  2. I would remove 'Q' from the name → clockBootTimeToDateTime
  3. just qlonglong clockBoottime, no const and &
meven updated this revision to Diff 65204.Sep 2 2019, 8:30 AM
meven marked an inline comment as done.

Review, remove incorrect doc

jgrulich accepted this revision.Sep 2 2019, 8:46 AM

Sorry, I missed that last one. Once it's fixed it's ready to go.

src/wirelessdevice.h
232

const QDateTime &timestamp

This revision is now accepted and ready to land.Sep 2 2019, 8:46 AM
meven updated this revision to Diff 65208.Sep 2 2019, 8:54 AM

Add a ref to the returned date

jgrulich accepted this revision.Sep 2 2019, 9:06 AM
meven edited the summary of this revision. (Show Details)Sep 2 2019, 9:18 AM
This revision was automatically updated to reflect the committed changes.
meven added a comment.Sep 2 2019, 9:20 AM

D23578 is the next step, but it is not in great shape.
To do things properly, it will require quite some changes I fear.

jgrulich added inline comments.Sep 4 2019, 10:52 AM
src/wirelessdevice.h
143

Thinking about it now, wouldn't be this wording better?

QDateTime lastScanRequestTime() const;

Maybe even without the "Time" at the end so it's consistent with "lastScan".

meven added inline comments.Sep 4 2019, 11:15 AM
src/wirelessdevice.h
143

I chose lastRequestScanTime to have RequestScan in the name, that is the function, it keeps the last time called from.

The Time suffix does not feel great indeed.

I can make an adjustment to lastRequestScan, I will adjust D23578 then

jgrulich added inline comments.Sep 4 2019, 11:35 AM
src/wirelessdevice.h
143

Please do, you can push it directly, it's just a function rename.