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

win_event_device_watcher wait before notify #12207

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

maloel
Copy link
Collaborator

@maloel maloel commented Sep 19, 2023

Events on removal arrive rapidly and so calling the callback for each is redundant but also wrong:

In testing, disconnection happened in two stages, triggering two events:

OLD group, before disconnection:
\\?\usb#vid_8086&pid_0b3a&mi_00#6&1e67ef5f&0&0000#{e5323777-f976-4f5b-9b55-b94699c46e44}\global
\\?\usb#vid_8086&pid_0b3a&mi_03#6&1e67ef5f&0&0003#{e5323777-f976-4f5b-9b55-b94699c46e44}\global

FIRST event after disconnection, NEW group:
\\?\usb#vid_8086&pid_0b3a&mi_03#6&1e67ef5f&0&0003#{e5323777-f976-4f5b-9b55-b94699c46e44}\global

OLD-NEW = removed
NEW-OLD = added

And operator-() is subtract_sets():
for each item on LEFT side:
add to result if none on RIGHT side is a superset of LEFT
In this case:
NEW is subset of OLD -> OLD is superset of NEW
OLD-NEW = 1 device removed
NEW-OLD = 0 device added

SECOND event after disconnection, OLD=previous new:
\\?\usb#vid_8086&pid_0b3a&mi_03#6&1e67ef5f&0&0003#{e5323777-f976-4f5b-9b55-b94699c46e44}\global

In this case:
NEW=empty
OLD-NEW = 1 device removed (again!?)
NEW-OLD = 0 device added

Solution is fairly simple, and was already done in udev device-watcher: wait a short while and only notify of changes after a brief time elapses post-event. Any new event refreshes the timer.

Events on removal arrive rapidly and so calling the callback for each is
redundant but also wrong:

In testing, disconnection happened in two stages, triggering two events:

OLD group, before disconnection:
\\?\usb#vid_8086&pid_0b3a&mi_00#6&1e67ef5f&0&0000#{e5323777-f976-4f5b-9b55-b94699c46e44}\global
\\?\usb#vid_8086&pid_0b3a&mi_03#6&1e67ef5f&0&0003#{e5323777-f976-4f5b-9b55-b94699c46e44}\global

FIRST event after disconnection, NEW group:
\\?\usb#vid_8086&pid_0b3a&mi_03#6&1e67ef5f&0&0003#{e5323777-f976-4f5b-9b55-b94699c46e44}\global

OLD-NEW = removed
NEW-OLD = added

And operator-() is subtract_sets():
    for each item on LEFT side:
        add to result if none on RIGHT side is a superset of LEFT
In this case:
    NEW is subset of OLD -> OLD is superset of NEW
    OLD-NEW = 1 device removed
    NEW-OLD = 0 device added

SECOND event after disconnection, OLD=previous NEW:
\\?\usb#vid_8086&pid_0b3a&mi_03#6&1e67ef5f&0&0003#{e5323777-f976-4f5b-9b55-b94699c46e44}\global

In this case:
    NEW=empty
    OLD-NEW = 1 device removed (again!?)
    NEW-OLD = 0 device added

Solution is fairly simple, and was already done in udev device-watcher:
wait a short while and only notify of changes after a brief time elapses
post-event. Any new event refreshes the timer.
@maloel maloel requested a review from Nir-Az September 19, 2023 06:16
@@ -311,30 +327,8 @@ namespace librealsense
if( p_hdr->dbch_devicetype != DBT_DEVTYP_DEVICEINTERFACE )
break;
auto data = reinterpret_cast<extra_data*>(GetWindowLongPtr(hWnd, GWLP_USERDATA));
auto next = data->_last;
std::wstring temp = reinterpret_cast<DEV_BROADCAST_DEVICEINTERFACE*>(lParam)->dbcc_name;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea what was the reason for all of this code?
And why it is not needed anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't very well comment about the first.
But it's not needed any more because we just get everything again fresh (after a delay), just as if we did query_devices. This is how udev does it, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably they were trying to save time...

Copy link
Collaborator

@Nir-Az Nir-Az left a comment

Choose a reason for hiding this comment

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

Overall, it looks like a great enhancement.
Just wondering about the code removal, too bad they didn't wrote comments because I have no idea what they did there.

@maloel maloel merged commit 8496930 into IntelRealSense:development Sep 19, 2023
@maloel maloel deleted the mf-watcher branch September 19, 2023 16:11
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.

2 participants