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

Remove memcpy in HWM & command_transfer #12622

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Remove memcpy in HWM & command_transfer #12622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a2dd334
remove one copy in HWM send_receive
maloel Jan 30, 2024
e6cd0e2
hwm remove 3 copies and many log msgs
maloel Jan 30, 2024
87396d6
remove now-unused hwmon_cmd
maloel Jan 30, 2024
33d46b1
add an error log to rs2_create_error
maloel Jan 30, 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
2 changes: 1 addition & 1 deletion src/api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ namespace librealsense



static void translate_exception(const char * name, std::string args, rs2_error ** error)
static void translate_exception(const char * name, std::string const & args, rs2_error ** error)
{
try { throw; }
catch (const librealsense_exception& e) { if (error) *error = rs2_create_error(e.what(), name, args.c_str(), e.get_exception_type() ); }
Expand Down
2 changes: 1 addition & 1 deletion src/ds/d500/hw_monitor_extended_buffers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ namespace librealsense
// - no buffer bigger than 1 KB is expected with the current command => work as normal hw_monitor::send method
// - buffer bigger than 1 KB expected to be received => iterate the hw_monitor::send method and append the results
// - buffer bigger than 1 KB expected to be sent => iterate the hw_monitor, while iterating over the input
std::vector<uint8_t> hw_monitor_extended_buffers::send(command cmd, hwmon_response* p_response, bool locked_transfer) const
std::vector<uint8_t> hw_monitor_extended_buffers::send(command const & cmd, hwmon_response* p_response, bool locked_transfer) const
{
hwm_buffer_type buffer_type = get_buffer_type(cmd);
if (buffer_type == hwm_buffer_type::standard)
Expand Down
2 changes: 1 addition & 1 deletion src/ds/d500/hw_monitor_extended_buffers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace librealsense
{}

virtual std::vector<uint8_t> send(std::vector<uint8_t> const& data) const override;
virtual std::vector<uint8_t> send(command cmd, hwmon_response* = nullptr, bool locked_transfer = false) const override;
virtual std::vector<uint8_t> send(command const & cmd, hwmon_response* = nullptr, bool locked_transfer = false) const override;

private:
int get_msg_length(command cmd) const;
Expand Down
24 changes: 17 additions & 7 deletions src/ds/ds-options.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,16 +550,26 @@ namespace librealsense
{
float rv = 0.f;
command cmd(ds::GETSUBPRESETID);
// if no subpreset is streaming, the firmware returns "ON_DATA_TO_RETURN" error
try {
auto res = _hwm.send(cmd);
// if a subpreset is streaming, checking this is the alternating emitter sub preset
if (res.size())
rv = (res[0] == ds::ALTERNATING_EMITTER_SUBPRESET_ID) ? 1.0f : 0.f;
try
{
hwmon_response response;
auto res = _hwm.send( cmd, &response ); // avoid the throw
OhadMeir marked this conversation as resolved.
Show resolved Hide resolved
switch( response )
{
case hwmon_response::hwm_NoDataToReturn:
// If no subpreset is streaming, the firmware returns "NO_DATA_TO_RETURN" error
break;
default:
// if a subpreset is streaming, checking this is the alternating emitter sub preset
if( res.size() )
rv = ( res[0] == ds::ALTERNATING_EMITTER_SUBPRESET_ID ) ? 1.0f : 0.f;
else
LOG_DEBUG( "alternating emitter query: " << hwmon_error_string( cmd, response ) );
break;
}
}
catch (...)
{
rv = 0.f;
}

return rv;
Expand Down
24 changes: 17 additions & 7 deletions src/hdr-config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,16 +228,26 @@ namespace librealsense
{
float rv = 0.f;
command cmd(ds::GETSUBPRESETID);
// if no subpreset is streaming, the firmware returns "ON_DATA_TO_RETURN" error
try {
auto res = _hwm.send(cmd);
// if a subpreset is streaming, checking this is the current HDR sub preset
if (res.size())
rv = (is_hdr_id(res[0])) ? 1.0f : 0.f;
try
{
hwmon_response response;
auto res = _hwm.send( cmd, &response ); // avoid the throw
OhadMeir marked this conversation as resolved.
Show resolved Hide resolved
switch( response )
{
case hwmon_response::hwm_NoDataToReturn:
// If no subpreset is streaming, the firmware returns "NO_DATA_TO_RETURN" error
break;
default:
// If a subpreset is streaming, checking this is the current HDR sub preset
if( res.size() )
rv = ( is_hdr_id( res[0] ) ) ? 1.0f : 0.f;
else
LOG_DEBUG( "hdr_config query: " << hwmon_error_string( cmd, response ) );
break;
}
}
catch (...)
{
rv = 0.f;
}

_is_enabled = (rv == 1.f);
Expand Down
60 changes: 26 additions & 34 deletions src/hw-monitor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,10 @@ namespace librealsense
return cmd;
}

void hw_monitor::execute_usb_command(uint8_t *out, size_t outSize, uint32_t & op, uint8_t * in,
void hw_monitor::execute_usb_command(uint8_t const *out, size_t outSize, uint32_t & op, uint8_t * in,
size_t & inSize, bool require_response) const
{
std::vector<uint8_t> out_vec(out, out + outSize);
auto res = _locked_transfer->send_receive(out_vec, 5000, require_response);
auto res = _locked_transfer->send_receive( out, outSize, 5000, require_response );

// read
if (require_response && in && inSize)
Expand Down Expand Up @@ -140,66 +139,59 @@ namespace librealsense

std::vector< uint8_t > hw_monitor::send( std::vector< uint8_t > const & data ) const
{
return _locked_transfer->send_receive(data);
return _locked_transfer->send_receive( data.data(), data.size() );
}

std::vector< uint8_t >
hw_monitor::send( command cmd, hwmon_response * p_response, bool locked_transfer ) const
hw_monitor::send( command const & cmd, hwmon_response * p_response, bool locked_transfer ) const
{
hwmon_cmd newCommand(cmd);
auto opCodeXmit = static_cast<uint32_t>(newCommand.cmd);
uint32_t const opCodeXmit = cmd.cmd;

hwmon_cmd_details details;
details.require_response = newCommand.require_response;
details.timeOut = newCommand.timeOut;

fill_usb_buffer(opCodeXmit,
newCommand.param1,
newCommand.param2,
newCommand.param3,
newCommand.param4,
newCommand.data,
newCommand.sizeOfSendCommandData,
details.sendCommandData.data(),
details.sizeOfSendCommandData);
details.require_response = cmd.require_response;
details.timeOut = cmd.timeout_ms;

fill_usb_buffer( opCodeXmit,
cmd.param1,
cmd.param2,
cmd.param3,
cmd.param4,
cmd.data.data(), // memcpy
std::min( (uint16_t)cmd.data.size(), HW_MONITOR_BUFFER_SIZE ),
details.sendCommandData.data(),
details.sizeOfSendCommandData );

if (locked_transfer)
{
return _locked_transfer->send_receive({ details.sendCommandData.begin(),details.sendCommandData.end() });
return _locked_transfer->send_receive( details.sendCommandData.data(), details.sendCommandData.size() );
}

send_hw_monitor_command(details);

// Error/exit conditions
if (p_response)
*p_response = hwm_Success;
if( !newCommand.require_response )
return std::vector<uint8_t>();

std::memcpy( newCommand.receivedOpcode, details.receivedOpcode.data(), 4 );
std::memcpy( newCommand.receivedCommandData,
details.receivedCommandData.data(),
details.receivedCommandDataLength );
newCommand.receivedCommandDataLength = details.receivedCommandDataLength;
if( ! cmd.require_response )
return {};

// endian?
auto opCodeAsUint32 = pack(details.receivedOpcode[3], details.receivedOpcode[2],
details.receivedOpcode[1], details.receivedOpcode[0]);
if (opCodeAsUint32 != opCodeXmit)
{
auto err_type = static_cast<hwmon_response>(opCodeAsUint32);
std::string err = hwmon_error_string(cmd, err_type);
LOG_DEBUG(err);
if (p_response)
//LOG_DEBUG(err); // too intrusive; may be an expected error
Copy link
Collaborator

Choose a reason for hiding this comment

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

You removed the line that initialize err

std::string err = hwmon_error_string(cmd, err_type);

Better to keep as comment or remove both, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I'll remove

if( p_response )
{
*p_response = err_type;
return std::vector<uint8_t>();
return {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to understand, why is this better than before? what happens behind the scene?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They're exactly the same, except the latter will still work if we change the return type of the function.

}
std::string err = hwmon_error_string( cmd, err_type );
throw invalid_value_exception(err);
}

return std::vector<uint8_t>(newCommand.receivedCommandData,
newCommand.receivedCommandData + newCommand.receivedCommandDataLength);
auto const pb = details.receivedCommandData.data();
return std::vector<uint8_t>( pb, pb + details.receivedCommandDataLength );
}

/*static*/ std::vector<uint8_t> hw_monitor::build_command(uint32_t opcode,
Expand Down
51 changes: 4 additions & 47 deletions src/hw-monitor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ namespace librealsense
{}

std::vector<uint8_t> send_receive(
const std::vector<uint8_t>& data,
uint8_t const * pb, size_t cb,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What pb & cp means?
Don't you prefer meaningful names?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How PB & CP are data and data_size? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's pretty common Hungarian notation for "pointer to bytes" and "count of bytes".

int timeout_ms = 5000,
bool require_response = true)
{
Expand All @@ -222,7 +222,7 @@ namespace librealsense
return strong_uvc->invoke_powered([&] ( platform::uvc_device & dev )
{
std::lock_guard<platform::uvc_device> lock(dev);
return _command_transfer->send_receive(data, timeout_ms, require_response);
return _command_transfer->send_receive(pb, cb, timeout_ms, require_response);
});
}

Expand Down Expand Up @@ -271,49 +271,6 @@ namespace librealsense
class hw_monitor
{
protected:
struct hwmon_cmd
{
uint8_t cmd;
int param1;
int param2;
int param3;
int param4;
uint8_t data[HW_MONITOR_BUFFER_SIZE];
int sizeOfSendCommandData;
long timeOut;
bool require_response;
uint8_t receivedCommandData[HW_MONITOR_BUFFER_SIZE];
size_t receivedCommandDataLength;
uint8_t receivedOpcode[4];

explicit hwmon_cmd(uint8_t cmd_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we use it?
Python maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope. No longer in use (I removed the only usage).

: cmd(cmd_id),
param1(0),
param2(0),
param3(0),
param4(0),
sizeOfSendCommandData(0),
timeOut(5000),
require_response(true),
receivedCommandDataLength(0)
{}


explicit hwmon_cmd(const command& cmd)
: cmd(cmd.cmd),
param1(cmd.param1),
param2(cmd.param2),
param3(cmd.param3),
param4(cmd.param4),
sizeOfSendCommandData(std::min((uint16_t)cmd.data.size(), HW_MONITOR_BUFFER_SIZE)),
timeOut(cmd.timeout_ms),
require_response(cmd.require_response),
receivedCommandDataLength(0)
{
std::memcpy( data, cmd.data.data(), sizeOfSendCommandData );
}
};

struct hwmon_cmd_details
{
bool require_response;
Expand All @@ -325,7 +282,7 @@ namespace librealsense
size_t receivedCommandDataLength;
};

void execute_usb_command(uint8_t *out, size_t outSize, uint32_t& op, uint8_t* in,
void execute_usb_command(uint8_t const *out, size_t outSize, uint32_t& op, uint8_t* in,
size_t& inSize, bool require_response) const;
static void update_cmd_details(hwmon_cmd_details& details, size_t receivedCmdLen, unsigned char* outputBuffer);
void send_hw_monitor_command(hwmon_cmd_details& details) const;
Expand All @@ -352,7 +309,7 @@ namespace librealsense
static command build_command_from_data(const std::vector<uint8_t> data);

virtual std::vector<uint8_t> send( std::vector<uint8_t> const & data ) const;
virtual std::vector<uint8_t> send( command cmd, hwmon_response * = nullptr, bool locked_transfer = false ) const;
virtual std::vector<uint8_t> send( command const & cmd, hwmon_response * = nullptr, bool locked_transfer = false ) const;
static std::vector<uint8_t> build_command(uint32_t opcode,
uint32_t param1 = 0,
uint32_t param2 = 0,
Expand Down
14 changes: 8 additions & 6 deletions src/platform/command-transfer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ namespace librealsense
{
public:
virtual std::vector<uint8_t> send_receive(
const std::vector<uint8_t>& data,
uint8_t const * pb,
size_t cb,
int timeout_ms = 5000,
bool require_response = true) = 0;

Expand All @@ -30,10 +31,7 @@ namespace librealsense
command_transfer_usb(const rs_usb_device& device) : _device(device) {}
~command_transfer_usb(){}

std::vector<uint8_t> send_receive(
const std::vector<uint8_t>& data,
int timeout_ms,
bool) override
std::vector< uint8_t > send_receive( uint8_t const * pb, size_t cb, int timeout_ms, bool ) override
{
auto intfs = _device->get_interfaces();
auto it = std::find_if(intfs.begin(), intfs.end(),
Expand All @@ -47,7 +45,11 @@ namespace librealsense
if (const auto& m = _device->open(hwm->get_number()))
{
uint32_t transfered_count = 0;
auto sts = m->bulk_transfer(hwm->first_endpoint(RS2_USB_ENDPOINT_DIRECTION_WRITE), const_cast<uint8_t*>(data.data()), static_cast<uint32_t>(data.size()), transfered_count, timeout_ms);
auto sts = m->bulk_transfer( hwm->first_endpoint( RS2_USB_ENDPOINT_DIRECTION_WRITE ),
const_cast< uint8_t * >( pb ),
static_cast< uint32_t >( cb ),
transfered_count,
timeout_ms );

if (sts != RS2_USB_STATUS_SUCCESS)
throw std::runtime_error("command transfer failed to execute bulk transfer, error: " + usb_status_to_string.at(sts));
Expand Down
10 changes: 5 additions & 5 deletions src/platform/uvc-option.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,24 +113,24 @@ const char* uvc_pu_option::get_description() const
}


std::vector<uint8_t> command_transfer_over_xu::send_receive(const std::vector<uint8_t>& data, int, bool require_response)
std::vector<uint8_t> command_transfer_over_xu::send_receive( uint8_t const * const pb, size_t const cb, int, bool require_response)
{
return _uvc.invoke_powered([this, &data, require_response]( platform::uvc_device & dev )
return _uvc.invoke_powered([this, pb, cb, require_response]( platform::uvc_device & dev )
{
std::vector<uint8_t> result;

std::lock_guard< platform::uvc_device > lock(dev);

if( data.size() > HW_MONITOR_BUFFER_SIZE )
if( cb > HW_MONITOR_BUFFER_SIZE )
{
LOG_ERROR( "XU command size is invalid" );
throw invalid_value_exception( rsutils::string::from()
<< "Requested XU command size " << std::dec << data.size()
<< "Requested XU command size " << std::dec << cb
<< " exceeds permitted limit " << HW_MONITOR_BUFFER_SIZE );
}

std::vector< uint8_t > transmit_buf( HW_MONITOR_BUFFER_SIZE, 0 );
std::copy( data.begin(), data.end(), transmit_buf.begin() );
std::copy( pb, pb + cb, transmit_buf.begin() );

if( ! dev.set_xu( _xu, _ctrl, transmit_buf.data(), static_cast< int >( transmit_buf.size() ) ) )
throw invalid_value_exception( rsutils::string::from()
Expand Down
2 changes: 1 addition & 1 deletion src/platform/uvc-option.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ class command_transfer_over_xu : public platform::command_transfer
uint8_t _ctrl;

public:
std::vector< uint8_t > send_receive( const std::vector< uint8_t > & data, int, bool require_response ) override;
std::vector< uint8_t > send_receive( uint8_t const * pb, size_t cb, int, bool require_response ) override;

command_transfer_over_xu( uvc_sensor & uvc, platform::extension_unit xu, uint8_t ctrl )
: _uvc( uvc )
Expand Down
1 change: 1 addition & 0 deletions src/rs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ struct rs2_error

rs2_error *rs2_create_error(const char* what, const char* name, const char* args, rs2_exception_type type) BEGIN_API_CALL
{
LOG_ERROR( "[" << name << "][" << rs2_exception_type_to_string( type ) << "] " << what << ": " << args );
OhadMeir marked this conversation as resolved.
Show resolved Hide resolved
return new rs2_error{ what, name, args, type };
}
NOEXCEPT_RETURN(nullptr, what, name, args, type)
Expand Down
5 changes: 4 additions & 1 deletion src/types.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ namespace librealsense
rs2_exception_type exception_type) noexcept
: librealsense_exception(msg, exception_type)
{
LOG_DEBUG("recoverable_exception: " << msg);
// This is too intrusive: some throws are completely valid and even expected (options-watcher queries all
// options, some of which may not be queryable) and this just dirties up the logs. It doesn't have the actual
// exception type, either.
//LOG_DEBUG("recoverable_exception: " << msg);
}

}
Loading