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

Update FW version display after FW update #13316

Conversation

noacoohen
Copy link
Contributor

Tracked by LRS-686

common/fw-update-helper.cpp Outdated Show resolved Hide resolved
@Nir-Az Nir-Az requested a review from maloel September 3, 2024 16:24
@Nir-Az
Copy link
Collaborator

Nir-Az commented Sep 3, 2024

@maloel can you help reviewing this PR?
Thanks

@maloel
Copy link
Collaborator

maloel commented Sep 4, 2024

This PR will work as long as the user holds the context from which the device was created.

E.g.:

ctx = context()
ctx.set_devices_changed_callback( on_devices_changed )
device = context().query_devices()[0]
device.hardware_reset()

This will not call on_devices_changed because it's on another context!!!

This may be OK: if we can agree that, for the D457 notifications to work, the user has to keep holding the context, then the PR is acceptable.

To make it so that the notification is sent to ALL contexts that may be listening, more extensive changes must be made: we need to take the invocation up to the level of the device-watcher and likely add an API to device_info.

@Nir-Az please see if the above is acceptable or whether we want to make more extensive changes.

src/ds/d400/d400-factory.cpp Outdated Show resolved Hide resolved
src/ds/d400/d400-factory.cpp Outdated Show resolved Hide resolved
src/ds/d400/d400-factory.cpp Outdated Show resolved Hide resolved
src/ds/d400/d400-factory.cpp Outdated Show resolved Hide resolved
@Nir-Az
Copy link
Collaborator

Nir-Az commented Sep 4, 2024

This PR will work as long as the user holds the context from which the device was created.

E.g.:

ctx = context()
ctx.set_devices_changed_callback( on_devices_changed )
device = context().query_devices()[0]
device.hardware_reset()

This will not call on_devices_changed because it's on another context!!!

This may be OK: if we can agree that, for the D457 notifications to work, the user has to keep holding the context, then the PR is acceptable.

To make it so that the notification is sent to ALL contexts that may be listening, more extensive changes must be made: we need to take the invocation up to the level of the device-watcher and likely add an API to device_info.

@Nir-Az please see if the above is acceptable or whether we want to make more extensive changes.

I'm OK with that,
It's much better than what we have today, and this is an internal enhancement so for now it's better to avoid massive changes IMO

@maloel
Copy link
Collaborator

maloel commented Sep 10, 2024

Notice there's an error in the CI

@Nir-Az Nir-Az merged commit 05777c5 into IntelRealSense:development Sep 11, 2024
20 checks passed
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.

3 participants