make hwmon_response product line dependent #13444

AviaAv · 2024-10-20T10:50:53Z

Tracked on: [LRS-1105]

Nir-Az · 2024-10-27T09:13:34Z

src/ds/d400/d400-device.h

        friend class d400_depth_sensor;

        std::shared_ptr<hw_monitor> _hw_monitor;
+        std::shared_ptr<ds::d400_hwmon_response_handler> hw_monitor_response = std::make_shared<ds::d400_hwmon_response_handler>();


Why here and not in the constructor?

Nir-Az · 2024-10-27T09:14:29Z

@OhadMeir please review

OhadMeir · 2024-10-27T13:16:08Z

src/hw-monitor.h


-    std::string hwmon_error_string( command const &, hwmon_response e );
+    typedef int32_t hwmon_response;
+    class base_hwmon_response_handler { // base class for different product number to implement responses


We don't have product numbers, you probably meant product ID, which also does not sound correct here. Rephrase to products or product lines

OhadMeir · 2024-10-27T13:16:36Z

src/hw-monitor.h

+    typedef int32_t hwmon_response;
+    class base_hwmon_response_handler { // base class for different product number to implement responses
+    public:
+        inline virtual std::string hwmon_error2str(int e) const = 0;


Virtual can be inlined. Remove inline keyword

OhadMeir · 2024-10-27T13:19:24Z

src/hw-monitor.h

+    class base_hwmon_response_handler { // base class for different product number to implement responses
+    public:
+        inline virtual std::string hwmon_error2str(int e) const = 0;
+        virtual hwmon_response hwmon_Success() const = 0;


Meaning is ambiguous, can be understood as is_successful or success_value.
According to the rest of the code it's the later, please rename.
Note - naming convention, should not capitalize after underscore.

OhadMeir · 2024-10-27T13:27:33Z

src/hw-monitor.h


-    std::string hwmon_error_string( command const &, hwmon_response e );
+    typedef int32_t hwmon_response;
+    class base_hwmon_response_handler { // base class for different product number to implement responses


On most places we name interfaces as XXX_interface. Consider renaming hwmon_response_handler_interface or hwmon_response_interface (are we a "handler"?).
Also can wrap typedef int32_t hwmon_response in the class namespace as hwmon_response_interface::type

OhadMeir · 2024-10-27T13:41:45Z

src/hw-monitor.h

-        explicit hw_monitor(std::shared_ptr<locked_transfer> locked_transfer)
-            : _locked_transfer(std::move(locked_transfer))
+        explicit hw_monitor(std::shared_ptr<locked_transfer> locked_transfer, std::shared_ptr<base_hwmon_response_handler> hwmon_response_handler)
+            : _locked_transfer(std::move(locked_transfer)), hwmon_response_handler(hwmon_response_handler)


Use _ as a prefix to member variables name

OhadMeir · 2024-10-27T14:49:29Z

src/ds/d400/d400-private.h

+        public:
+            enum d400_hwmon_response_names : int32_t
+            {
+                hwm_Success                         =  0,


As we discussed, can shorten the literal names because it's in a class namespace.
d400_hwmon_response::success

OhadMeir · 2024-10-27T15:05:59Z

src/hw-monitor.h

+
+        std::shared_ptr<base_hwmon_response_handler> hwmon_response_handler;
+
+        std::string hwmon_error_string(command const&, int e) const;


Should be hwmon_response not int

OhadMeir · 2024-10-27T15:11:32Z

src/hdr-config.cpp

                hwmon_response response;
                auto res = _hwm.send( cmd, &response );  // avoid the throw
-                switch( response )
+                if (response != _hwm.hwmon_response_handler->hwmon_Success()) // If no subpreset is streaming, the firmware returns "NO_DATA_TO_RETURN" error


This is not exactly equivalent to the former logic. Why was this changed?

I think it's becasue now we get the success value from a virtual function. I'm getting an error when trying to use _hwm.hwmon_response_handler->hwmon_Success() as a 'case' value ('this' can not be used in a constant expression)

Yes, but now you don't have any reference to hwmon_response::hwm_NoDataToReturn, is it not needed?
You can replace switch with some else if clauses, but here you have changed the logic. Just make sure its intended and works as expected.

oh I see, thanks, I will add no_data_to_return_opcode as a member to this class, like we did on alternating_emitter_option

OhadMeir · 2024-10-27T15:13:23Z

src/ds/ds-options.h

    {
    public:
-        alternating_emitter_option(hw_monitor& hwm, bool is_fw_version_using_id);
+        alternating_emitter_option(hw_monitor& hwm, bool is_fw_version_using_id, int no_data_to_return_opcode);


Use hwmon_response type, not int

OhadMeir · 2024-10-27T15:14:51Z

src/ds/d400/d400-private.h

+        class d400_hwmon_response_handler : public base_hwmon_response_handler
+        {
+        public:
+            enum d400_hwmon_response_names : int32_t


Here too, use the defined hwmon_response type

OhadMeir · 2024-10-27T15:17:04Z

src/ds/d400/d400-private.h

+        {
+        public:
+            enum d400_hwmon_response_names : int32_t
+            {


Rename d400_hwmon_response_names to opcodes. It will be under the class namespace.

OhadMeir · 2024-10-27T15:21:28Z

src/ds/d400/d400-device.h


        friend class d400_depth_sensor;

        std::shared_ptr<hw_monitor> _hw_monitor;


Member hw_monitor_response is not needed, create the shared object as you directly pass it into _hw_monitor.
Also, should be prefixed with _

OhadMeir · 2024-10-27T15:22:16Z

src/ds/d500/d500-private.h

+        class d500_hwmon_response_handler : public base_hwmon_response_handler
+        {
+        public:
+            enum d500_hwmon_response_names : int32_t


Same comments as d400

make hwmon_response product line dependent

011ffae

AviaAv requested a review from Nir-Az October 20, 2024 10:51

Nir-Az reviewed Oct 27, 2024

View reviewed changes

Nir-Az requested review from Nir-Az and OhadMeir and removed request for Nir-Az October 27, 2024 09:13

OhadMeir reviewed Oct 27, 2024

View reviewed changes

renaming types and small fixes

303c5af

OhadMeir approved these changes Oct 30, 2024

View reviewed changes

OhadMeir merged commit 57eed2d into IntelRealSense:development Oct 30, 2024
20 checks passed


		std::shared_ptr<base_hwmon_response_handler> hwmon_response_handler;

		std::string hwmon_error_string(command const&, int e) const;


		friend class d400_depth_sensor;

		std::shared_ptr<hw_monitor> _hw_monitor;

make hwmon_response product line dependent #13444

make hwmon_response product line dependent #13444

Uh oh!

Conversation

AviaAv commented Oct 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nir-Az commented Oct 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants