PR #12622 from Eran: Remove memcpy in HWM & command_transfer

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() ); }

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<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)

src/ds/d500/hw_monitor_extended_buffers.h

@@ -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;

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;

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);

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<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)
@@ -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
+            if( p_response )
             {
                 *p_response = err_type;
-                return std::vector<uint8_t>();
+                return {};
             }
+            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,

src/hw-monitor.h

@@ -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,
             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<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);
                 });
         }
 
@@ -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;
@@ -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;
@@ -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,

src/platform/command-transfer.h

@@ -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;
 
@@ -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(),
@@ -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));

src/platform/uvc-option.cpp

@@ -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()

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 )

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)

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);
     }
 
 }