RSDK-6632 - Typescript SDK throws if MotionConfiguration.obstacleDetectorsList is not specified

[nfranczak]

Steps to reproduce :

Typescript SDK encodeMotionConfiguration throws an exception if MotionConfiguration.obstacleDetectorsList is not specified

Expected behavior :

Typescript SDK allows all MotionConfiguration attributes to be optional

[nfranczak]

relates to viamrobotics/rdk#3574

[micheal-parks]

Are these necessary to coalesce? In other words, will this break somewhere if we do not send default values? My concern is that default values now potentially exist - and would need to be maintained - in multiple places (SDK and robot).

[nfranczak]

Are these necessary to coalesce? In other words, will this break somewhere if we do not send default values?... My concern is that default values now potentially exist - and would need to be maintained - in multiple places (SDK and robot).

Good catch! I will change this to be:

  if (obj.angularDegsPerSec) {
    result.setAngularDegsPerSec(obj.angularDegsPerSec);
  }

[micheal-parks]

It would be safer for these checks to do:

if ('positionPollingFrequencyHz' in obj) {
  result.setPositionPollingFrequencyHz(obj.positionPollingFrequencyHz)
}

The current check will evaluate to false if any of these props are zero, and therefore won't be set, which will cause unexpected behavior.

[nfranczak]

with this, when linting, I run into:

src/services/motion/types.ts(88,42): error TS2345: Argument of type 'number | undefined' is not assignable to parameter of type 'number'.
  Type 'undefined' is not assignable to type 'number'.

I will change the conditionals to be:

if (obj.positionPollingFrequencyHz !== undefined) {
    result.setPositionPollingFrequencyHz(obj.positionPollingFrequencyHz);
}

this way even if any of the props are zero or false they are set.

[micheal-parks]

That works!

[nicksanford]

How does this behave:

1. if obj is undefined?
2. if obstacleDetectorsList is not a member of obj?

[micheal-parks]

1. The parameter type for obj for this function states that it will never be undefined. If that's not the case, it should be an optional param, and the param should recieve a default value:

export const encodeMotionConfiguration = (
  obj?: MotionConfiguration = {}
): pb.MotionConfiguration =>

2. If it's not a member of this object, it will be assigned a default value of [].

[nfranczak]

if obj is undefined?

The parameter type for obj for this function states that it will never be undefined. If that's not the case, it should be an optional param, and the param should recieve a default value:

Within typescript-sdk/src/services/motion/client.ts for moveOnGlobe and moveOnMap we do

    if (motionConfig) {
      request.setMotionConfiguration(encodeMotionConfiguration(motionConfig));
    }

So I think it is safe to assume that obj will not be undefined entirely.

[micheal-parks]

In this case I'd actually recommend making this an optional param like I've done above. If all fields are optional then there's no reason the user should have to pass an empty object into this function.

[nicksanford]

@nfranczak Tests are not passing

[nfranczak]

@micheal-parks since I added this, do you think I should remove

    if (motionConfig) {
      request.setMotionConfiguration(encodeMotionConfiguration(motionConfig));
    }

from src/services/motion/client.ts?

[micheal-parks]

I think it still makes sense to not call this function if no config is provided, unless something will break if it's not called

[nicksanford]

Please confirm this change works with the RDK RC card post-merge (or ideally pre-merge if you can).

[nfranczak]

Please confirm this change works with the RDK RC card post-merge (or ideally pre-merge if you can).

@nicksanford @micheal-parks Would you know how to test this change pre-merge - is that possible?

[micheal-parks]

Pre-merge might be difficult with our current setup, I think post-merge is fine for now. All you'll have to do is:

1. bump the SDK version in this repo's package.json. That'll trigger our CI pipeline to ship a new version to NPM
2. install the new version in the RDK, use this method, and run npm run check. Nick S might be able to assist since he already started work here.

[micheal-parks]

Thanks for addressing my comments, good stuff!