From a2dd3343e65668e383389e40ddd3aff339bba541 Mon Sep 17 00:00:00 2001 From: Eran Date: Tue, 30 Jan 2024 12:45:56 +0200 Subject: [PATCH 1/4] remove one copy in HWM send_receive --- src/hw-monitor.cpp | 9 ++++----- src/hw-monitor.h | 6 +++--- src/platform/command-transfer.h | 14 ++++++++------ src/platform/uvc-option.cpp | 10 +++++----- src/platform/uvc-option.h | 2 +- 5 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/hw-monitor.cpp b/src/hw-monitor.cpp index baa42792f5..8ee90cc1a2 100644 --- a/src/hw-monitor.cpp +++ b/src/hw-monitor.cpp @@ -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 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) @@ -140,7 +139,7 @@ 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 > @@ -165,7 +164,7 @@ namespace librealsense 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); diff --git a/src/hw-monitor.h b/src/hw-monitor.h index 4fc0645071..e2b57976aa 100644 --- a/src/hw-monitor.h +++ b/src/hw-monitor.h @@ -204,7 +204,7 @@ namespace librealsense {} std::vector send_receive( - const std::vector& data, + uint8_t const * pb, size_t cb, int timeout_ms = 5000, bool require_response = true) { @@ -222,7 +222,7 @@ namespace librealsense return strong_uvc->invoke_powered([&] ( platform::uvc_device & dev ) { std::lock_guard lock(dev); - return _command_transfer->send_receive(data, timeout_ms, require_response); + return _command_transfer->send_receive(pb, cb, timeout_ms, require_response); }); } @@ -325,7 +325,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; diff --git a/src/platform/command-transfer.h b/src/platform/command-transfer.h index 26bf1e1cea..4c0cc45d0f 100644 --- a/src/platform/command-transfer.h +++ b/src/platform/command-transfer.h @@ -17,7 +17,8 @@ namespace librealsense { public: virtual std::vector send_receive( - const std::vector& data, + uint8_t const * pb, + size_t cb, int timeout_ms = 5000, bool require_response = true) = 0; @@ -30,10 +31,7 @@ namespace librealsense command_transfer_usb(const rs_usb_device& device) : _device(device) {} ~command_transfer_usb(){} - std::vector send_receive( - const std::vector& 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(), @@ -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(data.data()), static_cast(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)); diff --git a/src/platform/uvc-option.cpp b/src/platform/uvc-option.cpp index eec641b5fa..8f35909021 100644 --- a/src/platform/uvc-option.cpp +++ b/src/platform/uvc-option.cpp @@ -113,24 +113,24 @@ const char* uvc_pu_option::get_description() const } -std::vector command_transfer_over_xu::send_receive(const std::vector& data, int, bool require_response) +std::vector 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 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() diff --git a/src/platform/uvc-option.h b/src/platform/uvc-option.h index 58b7292fcc..67075c0ac0 100644 --- a/src/platform/uvc-option.h +++ b/src/platform/uvc-option.h @@ -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 ) From e6cd0e282308e7c39ac279eaf0ab2e632de6b36b Mon Sep 17 00:00:00 2001 From: Eran Date: Tue, 30 Jan 2024 09:07:56 +0200 Subject: [PATCH 2/4] hwm remove 3 copies and many log msgs --- src/ds/d500/hw_monitor_extended_buffers.cpp | 2 +- src/ds/d500/hw_monitor_extended_buffers.h | 2 +- src/ds/ds-options.cpp | 24 +++++++--- src/hdr-config.cpp | 24 +++++++--- src/hw-monitor.cpp | 51 +++++++++------------ src/hw-monitor.h | 2 +- src/types.cpp | 5 +- 7 files changed, 63 insertions(+), 47 deletions(-) diff --git a/src/ds/d500/hw_monitor_extended_buffers.cpp b/src/ds/d500/hw_monitor_extended_buffers.cpp index 469f5534d7..d00f8b4826 100644 --- a/src/ds/d500/hw_monitor_extended_buffers.cpp +++ b/src/ds/d500/hw_monitor_extended_buffers.cpp @@ -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 hw_monitor_extended_buffers::send(command cmd, hwmon_response* p_response, bool locked_transfer) const + std::vector 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) diff --git a/src/ds/d500/hw_monitor_extended_buffers.h b/src/ds/d500/hw_monitor_extended_buffers.h index 9d8f85cbe0..fad6b23759 100644 --- a/src/ds/d500/hw_monitor_extended_buffers.h +++ b/src/ds/d500/hw_monitor_extended_buffers.h @@ -26,7 +26,7 @@ namespace librealsense {} virtual std::vector send(std::vector const& data) const override; - virtual std::vector send(command cmd, hwmon_response* = nullptr, bool locked_transfer = false) const override; + virtual std::vector send(command const & cmd, hwmon_response* = nullptr, bool locked_transfer = false) const override; private: int get_msg_length(command cmd) const; diff --git a/src/ds/ds-options.cpp b/src/ds/ds-options.cpp index 7890c1641a..c105c42144 100644 --- a/src/ds/ds-options.cpp +++ b/src/ds/ds-options.cpp @@ -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 + 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; diff --git a/src/hdr-config.cpp b/src/hdr-config.cpp index fd8ec45f38..63f8cacf4b 100644 --- a/src/hdr-config.cpp +++ b/src/hdr-config.cpp @@ -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 + 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); diff --git a/src/hw-monitor.cpp b/src/hw-monitor.cpp index 8ee90cc1a2..8ce92fe40d 100644 --- a/src/hw-monitor.cpp +++ b/src/hw-monitor.cpp @@ -143,24 +143,23 @@ namespace librealsense } 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(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) { @@ -172,14 +171,8 @@ namespace librealsense // Error/exit conditions if (p_response) *p_response = hwm_Success; - if( !newCommand.require_response ) - return std::vector(); - - 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], @@ -187,18 +180,18 @@ namespace librealsense if (opCodeAsUint32 != opCodeXmit) { auto err_type = static_cast(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 + if( p_response ) { *p_response = err_type; - return std::vector(); + return {}; } + std::string err = hwmon_error_string( cmd, err_type ); throw invalid_value_exception(err); } - return std::vector(newCommand.receivedCommandData, - newCommand.receivedCommandData + newCommand.receivedCommandDataLength); + auto const pb = details.receivedCommandData.data(); + return std::vector( pb, pb + details.receivedCommandDataLength ); } /*static*/ std::vector hw_monitor::build_command(uint32_t opcode, diff --git a/src/hw-monitor.h b/src/hw-monitor.h index e2b57976aa..086225388e 100644 --- a/src/hw-monitor.h +++ b/src/hw-monitor.h @@ -352,7 +352,7 @@ namespace librealsense static command build_command_from_data(const std::vector data); virtual std::vector send( std::vector const & data ) const; - virtual std::vector send( command cmd, hwmon_response * = nullptr, bool locked_transfer = false ) const; + virtual std::vector send( command const & cmd, hwmon_response * = nullptr, bool locked_transfer = false ) const; static std::vector build_command(uint32_t opcode, uint32_t param1 = 0, uint32_t param2 = 0, diff --git a/src/types.cpp b/src/types.cpp index 50cd39748b..88696cf467 100644 --- a/src/types.cpp +++ b/src/types.cpp @@ -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); } } From 87396d6d88c7f9bbf0d31224fb21ac21bfb660ac Mon Sep 17 00:00:00 2001 From: Eran Date: Tue, 30 Jan 2024 13:14:29 +0200 Subject: [PATCH 3/4] remove now-unused hwmon_cmd --- src/hw-monitor.h | 43 ------------------------------------------- 1 file changed, 43 deletions(-) diff --git a/src/hw-monitor.h b/src/hw-monitor.h index 086225388e..a83e74b700 100644 --- a/src/hw-monitor.h +++ b/src/hw-monitor.h @@ -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) - : 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; From 33d46b13b924b133fc670555edb60427e030cb8c Mon Sep 17 00:00:00 2001 From: Eran Date: Tue, 30 Jan 2024 13:59:50 +0200 Subject: [PATCH 4/4] add an error log to rs2_create_error --- src/api.h | 2 +- src/rs.cpp | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/api.h b/src/api.h index b28b4e0c6a..6684d4fae6 100644 --- a/src/api.h +++ b/src/api.h @@ -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() ); } diff --git a/src/rs.cpp b/src/rs.cpp index 5ae3ee8dfc..d14210b9a3 100644 --- a/src/rs.cpp +++ b/src/rs.cpp @@ -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 ); return new rs2_error{ what, name, args, type }; } NOEXCEPT_RETURN(nullptr, what, name, args, type)