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

Enriched display [DEVINFRA-389] #1256

Closed
wants to merge 7 commits into from
Closed

Conversation

adrian-kong
Copy link
Contributor

@adrian-kong adrian-kong commented Nov 28, 2022

Generate additional enriched field display and friendly names

Introduces two functions:
Sbp::friendly_name()
Sbp::message_display():

friendly_name mainly follows replacing MSG_, _ and abbreviating some keywords, manual implementation also possible via yaml. Mainly autogenerated

message_display aims to produce some sort of debug / format message output. Can be manually implemented via yaml with custom regex grouping {{ field }} to denote field and {{@ field @}} to represent self.field

@adrian-kong adrian-kong marked this pull request as ready for review November 29, 2022 06:04
@adrian-kong adrian-kong requested review from notoriaga, silverjam and a team as code owners November 29, 2022 06:04
@@ -521,6 +523,7 @@ definitions:

- MSG_CELL_MODEM_STATUS:
id: 0x00BE
message_display: "{{@ signal_strength @}} dBm | {{@ signal_error_rate @}} %"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with a funky regex group capturing

abc{{X}} represents format!("abc{}", X)
abc{{@ X @}} represents format!("abc{}", self.X)

Basically {{@ ... @}} prepends self.
This allows some things that only requires fields to be simplified but might be bit hacky.

Missing functionality to do something like this format!("{:.2}", self.X)
Maybe TODO: add more parsing for some syntaxes like {{@ ... @}:.3} where we also retain {{}:.} some element after char :.

We can parse methods if there are by just calling them like {{@ some_method() @}} etc... but maybe really hacky to be inside yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be better documented somewhere than just in a PR comment.

What sorts of things can we do with the first one abc{{X}}? Since we are not using that one I think my preference would be to just have only 1 style abc{{ field_name }} that substitutes the corresponding field value in or maybe abc{{ @field_name }} in the style of ruby. Probably keep this as simple as possible until we come across new usecases.

Copy link
Contributor Author

@adrian-kong adrian-kong Nov 29, 2022

Choose a reason for hiding this comment

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

The first one could be used something like

{{@ posX @ / 10}}
format!({}, self.posX / 10)

There are cases where we do operations operations on these fields - which might turn out really hacky inside the yaml when it gets really complicated as something like

{{if @ bool @ { 5 } else { 10 }}}
format!("{}", if self.bool { 5 } else { 10 })

Maybe there is a better way since these format methods would turn out really funky codegen wise, but avoided manual implementations of message_display by these scripting formats

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But yea @field_name style might be nice but would be weird to decide where to terminate i.e.

{{@posX + @posY }} // requiring end char to be ' '
{{@posX+@posY}} // no ' ' termination character

*Having @field_name might be simpler but was unsure if '@' came up often so might swap to this style instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay I see. These YAMLs probably want to be language independent right, we would not be able to use message_display in the python library if it is a rust expression. I think until there are usecases for this it might be simpler to only allow simple string interpolation for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use {fmt} style interpolation in most languages, but it'd take some work, and it'd need to be done in a language neutral way, so we couldn't have embedded Rust/whatever syntax:

This feature would not work for languages that don't implement some variant of the {fmt} string interpolation.

Copy link
Contributor

@joelynch joelynch left a comment

Choose a reason for hiding this comment

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

This is good but I think it needs a few tweaks.

@@ -521,6 +523,7 @@ definitions:

- MSG_CELL_MODEM_STATUS:
id: 0x00BE
message_display: "{{@ signal_strength @}} dBm | {{@ signal_error_rate @}} %"
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be better documented somewhere than just in a PR comment.

What sorts of things can we do with the first one abc{{X}}? Since we are not using that one I think my preference would be to just have only 1 style abc{{ field_name }} that substitutes the corresponding field value in or maybe abc{{ @field_name }} in the style of ruby. Probably keep this as simple as possible until we come across new usecases.

@adrian-kong adrian-kong marked this pull request as draft December 7, 2022 22:41
@adrian-kong
Copy link
Contributor Author

Going to reopen this in smaller PR, have the more complex message_display() seperate

@adrian-kong
Copy link
Contributor Author

split into #1262

@adrian-kong adrian-kong closed this Dec 7, 2022
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.

3 participants