Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RSDK-9281] Add native type return to Flutter SDK DiscoverComponents #299

Merged
merged 4 commits into from
Nov 19, 2024

Conversation

hexbabe
Copy link
Member

@hexbabe hexbabe commented Nov 15, 2024

RSDK-9281

Add native type return to Flutter SDK DiscoverComponents

Tested locally

@hexbabe hexbabe requested a review from a team as a code owner November 15, 2024 21:55
Copy link
Member

@jckras jckras left a comment

Choose a reason for hiding this comment

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

LGTM. my only q/comment is who will be reading these results? looking at the photo you included for local testing, the results are a bit difficult for a user to interpret (it is hard to tell if the nested discovery objects are related to the top-level component?). however, if this is primarily for internal use, than I can understand priorities might be different

@hexbabe
Copy link
Member Author

hexbabe commented Nov 18, 2024

LGTM. my only q/comment is who will be reading these results? looking at the photo you included for local testing, the results are a bit difficult for a user to interpret (it is hard to tell if the nested discovery objects are related to the top-level component?). however, if this is primarily for internal use, than I can understand priorities might be different

Really good point. This is a point of discussion with AV and mobile right now. I think this is a good initial step though https://viaminc.slack.com/archives/C07GTSXSVEC/p1731708356210009?thread_ts=1731688631.621549&cid=C07GTSXSVEC

@kevin49999
Copy link
Member

kevin49999 commented Nov 18, 2024

LGTM. my only q/comment is who will be reading these results? looking at the photo you included for local testing, the results are a bit difficult for a user to interpret (it is hard to tell if the nested discovery objects are related to the top-level component?). however, if this is primarily for internal use, than I can understand priorities might be different

Really good point. This is a point of discussion with AV and mobile right now. I think this is a good initial step though https://viaminc.slack.com/archives/C07GTSXSVEC/p1731708356210009?thread_ts=1731688631.621549&cid=C07GTSXSVEC

I do still agree with this point, on the frontend right now I'm playing around with getting this to something useable and it's tricky. The json string for the cameras in the results looks like this:

{
  "fields": {
    "cameras": {
      "Kind": {
        "ListValue": {
          "values": [
            {
              "Kind": {
                "StructValue": {
                  "fields": {
                    "rtsp_urls": {
                      "Kind": {
                        "ListValue": {}
                      }
                    },
                    "manufacturer": {
                      "Kind": {
                        "StringValue": "ONVIF_ICAMERA"
                      }
                    },
                    "model": {
                      "Kind": {
                        "StringValue": "F26C_IR_c3"
                      }
                    },
                    "serial_number": {
                      "Kind": {
                        "StringValue": "EF00000005087E34"
                      }
                    }
                  }
                }
              }
            },
            {
              "Kind": {
                "StructValue": {
                  "fields": {
                    "manufacturer": {
                      "Kind": {
                        "StringValue": "Amcrest"
                      }
                    },
                    "model": {
                      "Kind": {
                        "StringValue": "IP5M-T1179EW-AI-V3"
                      }
                    },
                    "serial_number": {
                      "Kind": {
                        "StringValue": "AMC1083EC3C8118218"
                      }
                    },
                    "rtsp_urls": {
                      "Kind": {
                        "ListValue": {
                          "values": [
                            {
                              "Kind": {
                                "StringValue": "rtsp://admin:[email protected]:554/cam/realmonitor?channel=1&subtype=0&unicast=true&proto=Onvif"
                              }
                            },
                            {
                              "Kind": {
                                "StringValue": "rtsp://admin:[email protected]:554/cam/realmonitor?channel=1&subtype=1&unicast=true&proto=Onvif"
                              }
                            }
                          ]
                        }
                      }
                    }
                  }
                }
              }
            },
            {
              "Kind": {
                "StructValue": {
                  "fields": {
                    "serial_number": {
                      "Kind": {
                        "StringValue": "EF0000000509A87D"
                      }
                    },
                    "rtsp_urls": {
                      "Kind": {
                        "ListValue": {}
                      }
                    },
                    "manufacturer": {
                      "Kind": {
                        "StringValue": "A_ONVIF_CAMERA"
                      }
                    },
                    "model": {
                      "Kind": {
                        "StringValue": "YMA42P_IR_N_AF"
                      }
                    }
                  }
                }
              }
            }
          ]
        }
      }
    }
  }
}

After calling toMap() all those extra keys like "Kind", "StringValue", "ListValue" make it tough to decode.

Though talking with the mobile team, there could be some easy step I'm missing

@njooma
Copy link
Member

njooma commented Nov 18, 2024

Q: why use native type? is it because the results are even more difficult to read as raw proto?

@kevin49999
Copy link
Member

Q: why use native type? is it because the results are even more difficult to read as raw proto?

If there was a proto for the results / a result maybe not? 🤔

@hexbabe
Copy link
Member Author

hexbabe commented Nov 18, 2024

Q: why use native type? is it because the results are even more difficult to read as raw proto?

If there was a proto for the results / a result maybe not? 🤔

Clarification: you mean if results wasn't a list of generic pb structs?

@kevin49999
Copy link
Member

kevin49999 commented Nov 18, 2024

Q: why use native type? is it because the results are even more difficult to read as raw proto?

If there was a proto for the results / a result maybe not? 🤔

Clarification: you mean if results wasn't a list of generic pb structs?

Exactly, sorry if my wording is off I haven't really worked w/ proto buffers / grpc

@kevin49999
Copy link
Member

Q: why use native type? is it because the results are even more difficult to read as raw proto?

If there was a proto for the results / a result maybe not? 🤔

Clarification: you mean if results wasn't a list of generic pb structs?

Exactly, sorry if my wording is off I haven't really worked w/ proto buffers / grpc

Though to provide more context, in the current app we're working on, when working with a robotConfig it's a bit easier to work with when converted to a map (and it's a pb struct)

@hexbabe
Copy link
Member Author

hexbabe commented Nov 18, 2024

The question that is currently blocking this PR is: can we make this even better for mobile?

And it looks like the answer is no unless we change the proto

if results wasn't a list of generic pb structs?

OK with me merging this? @kevin49999 @jckras @njooma

@njooma
Copy link
Member

njooma commented Nov 18, 2024

Let's not merge yet unless it's blocking something. There should be a way to fix the nested struct keys

@hexbabe
Copy link
Member Author

hexbabe commented Nov 19, 2024

Please see new util handler to unwrap nested "Kind" and "fields" wrappers

See the new output here

@hexbabe hexbabe requested review from njooma and removed request for njooma and stuqdog November 19, 2024 17:16
@hexbabe
Copy link
Member Author

hexbabe commented Nov 19, 2024

Further conversations were had and we are not going forward with a unwrapper https://viaminc.slack.com/archives/C07GTSXSVEC/p1732050452559639?thread_ts=1731688631.621549&cid=C07GTSXSVEC

@hexbabe hexbabe merged commit 68f593b into viamrobotics:main Nov 19, 2024
4 checks passed
@hexbabe hexbabe deleted the RSDK-9281 branch November 19, 2024 21:32
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