D500 - retry GET_GVD #12805
D500 - retry GET_GVD #12805
Conversation
(cherry picked from commit 8ff27ee0806a2110a771cc40215c05b224a41f03)
|
Main change is adding the retry mechanism, |
src/hw-monitor.cpp
Outdated
| { | ||
| // If we get an error code that match to the error code defined as require retry, | ||
| // we will retry the command until it succeed or we reach a timeout | ||
| bool should_retry = retry_error_codes && ! retry_error_codes->empty() |
There was a problem hiding this comment.
!empty is redundant with find() != end()
src/hw-monitor.cpp
Outdated
| auto data = send( command, &p_response ); | ||
| if( p_response != hwm_Success ) | ||
| { | ||
| // If we get an error code that match to the error code defined as require retry, |
There was a problem hiding this comment.
This is fine, but I still don't understand WHY we would have to retry a GVD command (specifically, or generally any command).
There was a problem hiding this comment.
As I explained, this is the flow that was agreed between SW & FW.
Origin from other HW not being ready yet while FW is already ready.
GVD contains other HW information as well, for more info please contact the architect
src/hw-monitor.h
Outdated
| @@ -1,50 +1,27 @@ | |||
| // License: Apache 2.0. See LICENSE file in root directory. | |||
| // Copyright(c) 2015 Intel Corporation. All Rights Reserved. | |||
| // Copyright(c) 2024 Intel Corporation. All Rights Reserved. | |||
src/hw-monitor.cpp
Outdated
| && retry_error_codes->find( p_response ) != retry_error_codes->end(); | ||
| if( should_retry ) | ||
| { | ||
| static constexpr size_t RETRIES = 50; |
There was a problem hiding this comment.
I don't see why we need static here?
src/hw-monitor.cpp
Outdated
| auto data = send(command); | ||
| hwmon_response p_response = hwmon_response::hwm_Unknown; | ||
| auto data = send( command, &p_response ); | ||
| if( p_response != hwm_Success ) |
There was a problem hiding this comment.
Looks like this if is exra... you can just put the inside block here.
There was a problem hiding this comment.
Thought it will be more efficient, but NVM, will change.
src/hw-monitor.cpp
Outdated
| if( should_retry ) | ||
| { | ||
| static constexpr size_t RETRIES = 50; | ||
| for( int i = 0; i < RETRIES; ++i ) |
There was a problem hiding this comment.
If you're gonna check the termination inside, seems extra to do it here. Just do while( true ) and increment inside?
There was a problem hiding this comment.
Sorry, I don't trust while (true)
In the future someone can change something and it will cause the application to stuck.
Prefer to keep as is.
| @@ -435,8 +435,15 @@ namespace librealsense | |||
| bool advanced_mode = false; | |||
| bool usb_modality = true; | |||
| group_multiple_fw_calls(depth_sensor, [&]() { | |||
|
|
|||
| const int HW_NOT_READY_ERR_CODE = -3; | |||
And why would this happen? Doesn't seem valid... |
src/hw-monitor.cpp
Outdated
| << "error in querying GVD, error:" | ||
| << hwmon_error2str( p_response ) ); | ||
| LOG_WARNING( "GVD not ready - retrying GET_GVD command" ); | ||
| std::this_thread::sleep_for( std::chrono::milliseconds( 100 ) ); |
There was a problem hiding this comment.
Shouldn't this be before line 247 (which, the first time, has no delay before it)?
This requirement comes from our SW architect, |
(cherry picked from commit cd3fd76fbdbc87c8da3f9678df307f67f585ed01)
|
Closed until final solution is defined. |
On D500 device, if the device is not ready after USB enumeration it will return NOT_READY error code to GET_GVD command.
This PR add retries when this happens on D500 devices
Note: also done some includes cleanup and renames
Tracked on [RSDEV-1826)]