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

Add widget API #1503

Merged
merged 7 commits into from
Jan 13, 2025
Merged

Conversation

Williangalvani
Copy link
Member

No description provided.

@Williangalvani Williangalvani force-pushed the widget_api branch 9 times, most recently from bc86de2 to 07b912b Compare December 17, 2024 16:48
@Williangalvani Williangalvani force-pushed the widget_api branch 11 times, most recently from e1d4e3c to cb52d94 Compare December 18, 2024 00:18
@Williangalvani Williangalvani changed the title [WIP] add widget api Add widget api Dec 18, 2024
@Williangalvani Williangalvani marked this pull request as ready for review December 18, 2024 00:19
{
echo '{
"name": "@bluerobotics/cockpit-api",
"version": "'${{ github.ref_name }}'",
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should not use the cockpit version here. thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't. Cockpit has new releases every week, while the API will probably be updated every few months.

@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Dec 19, 2024
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

I have put some comments here and there, but in general I really like the solution and loved that you added support for Ardupilot variables in the data-lake!

@ES-Alexander
Copy link
Contributor

This is presumably out of scope for this PR, but would this Cockpit API be the basis for supporting widgets registering their own Actions (per #609) in future? :-)

@rafaellehmkuhl
Copy link
Member

This is presumably out of scope for this PR, but would this Cockpit API be the basis for supporting widgets registering their own Actions (per #609) in future? :-)

Yes, the idea is to put their metadata in the same json.

@Williangalvani
Copy link
Member Author

This is presumably out of scope for this PR, but would this Cockpit API be the basis for supporting widgets registering their own Actions (per #609) in future? :-)

Yes, the idea is to put their metadata in the same json.

And to allow doing it programatically as we extend the API

@Williangalvani Williangalvani force-pushed the widget_api branch 3 times, most recently from 505a55a to 6c17b99 Compare January 6, 2025 16:36
if (typeof data !== 'string' && typeof data !== 'number') return

if (dataLakeVariableInfo[id] === undefined) {
createDataLakeVariable(new DataLakeVariable(id, id, typeof data === 'string' ? 'string' : 'number'))
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep this? I feel like it creates an incentive for people not to explicitly register their variables.

With the idea of also adding unit information to the variables, this will become more and more of a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can make it happen only in ardupilot.ts for the mavlink data. I assume we don't want to register all mavlink data manually

Copy link
Member

Choose a reason for hiding this comment

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

Agree, but in this case, to enforce the interface and so we can improve it in the future without having to change a lot of places in the code, I think it would be better to put this "type inferring" in the variable registration and register explicitly in the ardupilot file, even if without the type. It would just be this if clause, but in the ardupilot file instead of the data-lake itself. What do you think?

@rafaellehmkuhl rafaellehmkuhl merged commit 0f2e181 into bluerobotics:master Jan 13, 2025
11 checks passed
@rafaellehmkuhl rafaellehmkuhl changed the title Add widget api Add widget API Jan 15, 2025
@ES-Alexander ES-Alexander added docs-in-progress Included in an open docs PR docs-minimal See docs PR for suggested improvements and removed docs-needed Change needs to be documented docs-in-progress Included in an open docs PR labels Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-minimal See docs PR for suggested improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants