Skip to content

[RSDK-12899] Fix set device properties#64

Closed
Sebastian Munoz (SebastianMunozP) wants to merge 8 commits into
viam-modules:mainfrom
SebastianMunozP:fix-get-device-properties
Closed

[RSDK-12899] Fix set device properties#64
Sebastian Munoz (SebastianMunozP) wants to merge 8 commits into
viam-modules:mainfrom
SebastianMunozP:fix-get-device-properties

Conversation

@SebastianMunozP

@SebastianMunozP Sebastian Munoz (SebastianMunozP) commented Feb 3, 2026

Copy link
Copy Markdown
Contributor

This PR fixes a crash on the get_device_properties do_command, caused by a missing continue statement on get_device_properties handling.

This issue happens when the user calls get_device_properties with a non-struct payload, such as this:

{
  "set_device_properties": ""
}

With this fix, these kind of calls are correctly handled producing the following error

{
  "error": "calling set_device_properties on non-struct properties"
}

We are also checking that all settings being modified can actually be modified before doing it

This works OK:

{
  "set_device_properties":   
  {"OB_PROP_DEPTH_NOISE_REMOVAL_FILTER_MAX_SPECKLE_SIZE_INT": {
    "name": "OB_PROP_DEPTH_NOISE_REMOVAL_FILTER_MAX_SPECKLE_SIZE_INT",
    "type": "int",
    "max": 1000,
    "current": 1000,
    "id": 41,
    "default": 1000,
    "min": 1,
    "permission": "read_write"
  },
  "OB_PROP_DEPTH_ALIGN_HARDWARE_MODE_INT": {
    "current": 2,
    "min": 0,
    "max": 5,
    "permission": "read_write",
    "default": 0,
    "name": "OB_PROP_DEPTH_ALIGN_HARDWARE_MODE_INT",
    "id": 63,
    "type": "int"
  }
  }

This does not (missing current field), no fields are modified

{
  "set_device_properties":   
  {"OB_PROP_DEPTH_NOISE_REMOVAL_FILTER_MAX_SPECKLE_SIZE_INT": {
    "name": "OB_PROP_DEPTH_NOISE_REMOVAL_FILTER_MAX_SPECKLE_SIZE_INT",
    "type": "int",
    "max": 1000,    "id": 41,
    "default": 1000,
    "min": 1,
    "permission": "read_write"
  },
  "OB_PROP_DEPTH_ALIGN_HARDWARE_MODE_INT": {
    "current": 5,
    "min": 0,
    "max": 5,
    "permission": "read_write",
    "default": 0,
    "name": "OB_PROP_DEPTH_ALIGN_HARDWARE_MODE_INT",
    "id": 63,
    "type": "int"
  }
  }
}

@SebastianMunozP Sebastian Munoz (SebastianMunozP) changed the title Fix get device properties [RSDK-12899] Fix get device properties Feb 3, 2026
@hexbabe

Copy link
Copy Markdown
Contributor

PR title is inaccurate. This fix is for set device properties

Comment thread src/module/device_control.hpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is dead code btw

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want to skip with continue or error out here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, changing this to error out, also, since this is intended to modify multiple settings of the camera at the same time, I'm adding a 2 phase approach, in which first we check if all changes can be applied, and only when we are sure, we apply them,

@seanavery Sean Pollock (seanavery) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

Do we want to add a test to repro the mentioned error->fix in the description?

@SebastianMunozP Sebastian Munoz (SebastianMunozP) changed the title [RSDK-12899] Fix get device properties [RSDK-12899] Fix set device properties Feb 3, 2026
@10zingpd

10zingpd commented May 4, 2026

Copy link
Copy Markdown

Sean Pollock (@seanavery) anything worth merging here?

@10zingpd

10zingpd commented Jun 8, 2026

Copy link
Copy Markdown

Sean Pollock (@seanavery) should we just close this?

@seanavery Sean Pollock (seanavery) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

validation and crash fix already handled in #70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants