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.

Sign up for GitHub

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

D500 - retry GET_GVD #12805

Closed
wants to merge 2 commits into from
Closed

D500 - retry GET_GVD #12805

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae0719d
retry GVD command on not ready error
Nir-Az Mar 25, 2024
17bb2f2
CR updates
Nir-Az Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/ds/d500/d500-device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs explanation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will add a comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

const std::set< int32_t > gvd_retry_errors{ HW_NOT_READY_ERR_CODE };

_hw_monitor->get_gvd( gvd_buff.size(),
gvd_buff.data(),
ds::fw_cmd::GVD,
&gvd_retry_errors );

_hw_monitor->get_gvd(gvd_buff.size(), gvd_buff.data(), ds::fw_cmd::GVD);
get_gvd_details(gvd_buff, &gvd_parsed_fields);

_device_capabilities = ds_caps::CAP_ACTIVE_PROJECTOR | ds_caps::CAP_RGB_SENSOR | ds_caps::CAP_IMU_SENSOR |
Expand Down
44 changes: 36 additions & 8 deletions src/hw-monitor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
// 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.

#include "hw-monitor.h"
#include "types.h"
#include <iomanip>
#include <limits>
#include <sstream>
#include <rsutils/string/from.h>


static inline uint32_t pack( uint8_t c0, uint8_t c1, uint8_t c2, uint8_t c3 )
Expand All @@ -28,7 +27,7 @@ namespace librealsense
void hw_monitor::fill_usb_buffer(int opCodeNumber, int p1, int p2, int p3, int p4,
uint8_t const * data, int dataLength, uint8_t* bufferToSend, int& length)
{
auto preHeaderData = IVCAM_MONITOR_MAGIC_NUMBER;
auto preHeaderData = HW_MONITOR_MAGIC_NUMBER;

uint8_t* writePtr = bufferToSend;
auto header_size = 4;
Expand Down Expand Up @@ -205,7 +204,7 @@ namespace librealsense
int length;
std::vector<uint8_t> result;
size_t length_of_command_with_data = dataLength + size_of_command_without_data;
auto init_size = (length_of_command_with_data > IVCAM_MONITOR_MAX_BUFFER_SIZE) ? length_of_command_with_data : IVCAM_MONITOR_MAX_BUFFER_SIZE;
auto init_size = (length_of_command_with_data > HW_MONITOR_MAX_BUFFER_SIZE) ? length_of_command_with_data : HW_MONITOR_MAX_BUFFER_SIZE;
result.resize(init_size);
fill_usb_buffer(opcode, param1, param2, param3, param4, data, static_cast<int>(dataLength), result.data(), length);
result.resize(length);
Expand All @@ -226,10 +225,39 @@ namespace librealsense
}


void hw_monitor::get_gvd(size_t sz, unsigned char* gvd, uint8_t gvd_cmd) const
void hw_monitor::get_gvd( size_t sz,
unsigned char * gvd,
uint8_t gvd_cmd,
const std::set< int32_t > * retry_error_codes ) const
{
command command(gvd_cmd);
auto data = send(command);
hwmon_response p_response = hwmon_response::hwm_Unknown;
auto data = send( command, &p_response );
if( p_response != hwm_Success )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this if is exra... you can just put the inside block here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thought it will be more efficient, but NVM, will change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

{
// If we get an error code that match to the error code defined as require retry,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine, but I still don't understand WHY we would have to retry a GVD command (specifically, or generally any command).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

// we will retry the command until it succeed or we reach a timeout
bool should_retry = retry_error_codes && ! retry_error_codes->empty()
Copy link
Collaborator

Choose a reason for hiding this comment

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

!empty is redundant with find() != end()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

&& retry_error_codes->find( p_response ) != retry_error_codes->end();
if( should_retry )
{
static constexpr size_t RETRIES = 50;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see why we need static here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

for( int i = 0; i < RETRIES; ++i )
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're gonna check the termination inside, seems extra to do it here. Just do while( true ) and increment inside?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

{
data = send( command, &p_response );
maloel marked this conversation as resolved.
Show resolved Hide resolved
if( p_response == hwm_Success )
break;
// If we failed after 'RETRIES' retries or it is less `RETRIES` and the error
// code is not in the retry list than , raise an exception
if( i >= ( RETRIES - 1 ) || retry_error_codes->find( p_response ) == retry_error_codes->end() )
throw io_exception( rsutils::string::from()
<< "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 ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be before line 247 (which, the first time, has no delay before it)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

}
}
}
auto minSize = std::min(sz, data.size());
std::memcpy( gvd, data.data(), minSize );
}
Expand Down
52 changes: 16 additions & 36 deletions src/hw-monitor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

2015-24

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


#pragma once

#include "uvc-sensor.h"
#include <mutex>
#include "platform/command-transfer.h"
#include "uvc-sensor.h"
#include <string>
#include <algorithm>
#include <vector>
#include <set>
#include <mutex>


namespace librealsense
{
const uint8_t IV_COMMAND_FIRMWARE_UPDATE_MODE = 0x01;
const uint8_t IV_COMMAND_GET_CALIBRATION_DATA = 0x02;
const uint8_t IV_COMMAND_LASER_POWER = 0x03;
const uint8_t IV_COMMAND_DEPTH_ACCURACY = 0x04;
const uint8_t IV_COMMAND_ZUNIT = 0x05;
const uint8_t IV_COMMAND_LOW_CONFIDENCE_LEVEL = 0x06;
const uint8_t IV_COMMAND_INTENSITY_IMAGE_TYPE = 0x07;
const uint8_t IV_COMMAND_MOTION_VS_RANGE_TRADE= 0x08;
const uint8_t IV_COMMAND_POWER_GEAR = 0x09;
const uint8_t IV_COMMAND_FILTER_OPTION = 0x0A;
const uint8_t IV_COMMAND_VERSION = 0x0B;
const uint8_t IV_COMMAND_CONFIDENCE_THRESHHOLD= 0x0C;

const uint8_t IVCAM_MONITOR_INTERFACE = 0x4;
const uint8_t IVCAM_MONITOR_ENDPOINT_OUT = 0x1;
const uint8_t IVCAM_MONITOR_ENDPOINT_IN = 0x81;
const uint8_t IVCAM_MIN_SUPPORTED_VERSION = 13;
const uint8_t IVCAM_MONITOR_HEADER_SIZE = (sizeof(uint32_t) * 6);
const uint8_t NUM_OF_CALIBRATION_PARAMS = 100;
const uint8_t PARAMETERS2_BUFFER_SIZE = 50;
const uint8_t SIZE_OF_CALIB_HEADER_BYTES = 4;
const uint8_t NUM_OF_CALIBRATION_COEFFS = 64;

const uint16_t MAX_SIZE_OF_CALIB_PARAM_BYTES = 800;
const uint16_t SIZE_OF_CALIB_PARAM_BYTES = 512;
const uint16_t IVCAM_MONITOR_MAGIC_NUMBER = 0xcdab;
const uint16_t IVCAM_MONITOR_MAX_BUFFER_SIZE = 1024;
const uint16_t IVCAM_MONITOR_MUTEX_TIMEOUT = 3000;
const uint16_t HW_MONITOR_COMMAND_SIZE = 1000;
const uint16_t HW_MONITOR_BUFFER_SIZE = 1024;
const uint16_t HW_MONITOR_DATA_SIZE_OFFSET = 1020;
const uint16_t SIZE_OF_HW_MONITOR_HEADER = 4;

const uint16_t HW_MONITOR_MAGIC_NUMBER = 0xcdab;
const uint16_t HW_MONITOR_MAX_BUFFER_SIZE = 1024;
const uint16_t HW_MONITOR_MUTEX_TIMEOUT = 3000;
const uint16_t HW_MONITOR_COMMAND_SIZE = 1000;
const uint16_t HW_MONITOR_BUFFER_SIZE = 1024;
const uint16_t HW_MONITOR_DATA_SIZE_OFFSET = 1020;
const uint16_t SIZE_OF_HW_MONITOR_HEADER = 4;

class uvc_sensor;

Expand Down Expand Up @@ -318,7 +295,10 @@ namespace librealsense
uint8_t const * data = nullptr,
size_t dataLength = 0);

void get_gvd(size_t sz, unsigned char* gvd, uint8_t gvd_cmd) const;
void get_gvd( size_t sz,
unsigned char * gvd,
uint8_t gvd_cmd,
const std::set< int32_t > * retry_error_codes = nullptr ) const;

template<typename T>
std::string get_firmware_version_string( const std::vector< uint8_t > & buff,
Expand Down
Loading