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

Added initial CMS framework (CRSF Only) #339

Merged
merged 1 commit into from
Jun 21, 2020

Conversation

codecae
Copy link
Member

@codecae codecae commented May 10, 2020

Initial CMS framework and tool script. Currently for CRSF only.

Blocked by CMS over CRSF Compression PR found here

@0crap
Copy link
Contributor

0crap commented May 11, 2020

Will this go into lua-scripts 1.5.0?
Because the CMS over CRSF Compression PR is set to Milestone 4.3

@codecae
Copy link
Member Author

codecae commented May 11, 2020

Not likely. Can't have this without the other PR. Didn't make the cut for 4.2 due to timing on my behalf.

@codecae
Copy link
Member Author

codecae commented May 11, 2020

Also need to work in screen definitions for hi-res radios. (Horus, t16, etc)

@codecae
Copy link
Member Author

codecae commented May 11, 2020

Added support for 480x272 screens (Jumper, Horus, etc)

@mikeller mikeller added this to the 1.6 milestone Jun 7, 2020
Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

Looks good to me, only a couple of minor remarks.

local dest = {}
local rpt = false
local c = nil
for i=1, #buf do
Copy link
Member

Choose a reason for hiding this comment

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

Coding standard is to have spaces around operators.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will address

@@ -0,0 +1,120 @@
local _ = {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to name this _? I thought convention in lua was to use _ for unused values

Copy link
Member Author

Choose a reason for hiding this comment

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

This was just a convenient way of bucketing constants into a single reference object, while limiting keystrokes and improving readability. Open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

CONST?

left = 156,
},
exit = {
event = EVT_VIRTUAL_EXIT
Copy link
Member

Choose a reason for hiding this comment

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

Seems to be the same everywhere - does this need to be configured per screen size, now that we have virtual events?

Copy link
Member

Choose a reason for hiding this comment

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

It would be great if cmsConfig could be loaded only for the CMS version since it's not needed for the MSP version.

Copy link
Member Author

Choose a reason for hiding this comment

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

can they be called separate versions if they're based on the same framework?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mikeller this virtual event is kind of an assertion, as I don't have all of the hardware to confirm usability in this context. I'd like to have this tested on multiple platforms to see if it can truly be unified.

Copy link
Member Author

Choose a reason for hiding this comment

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

@klutvott123 would the suggestion then be to have radiosMsp and radiosCms? I feel like having them in one object is a little more DRY in nature.

Copy link
Member Author

@codecae codecae Jun 7, 2020

Choose a reason for hiding this comment

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

would it be better to put the MSP related attributes in a nested msp table and the cms related attributes into a nested cms table (instead of cmsConfig)? While cleaner, it would be quite disruptive to the rest of the codebase.

Copy link
Member

Choose a reason for hiding this comment

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

@codecae I'm using the word "version" loosely 😁. I was thinking maybe pass a variable to radios.lua when executing it and put cms or msp stuff into the radio table based on that.

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 added an update to separate the msp vs cms attributes in the radio config. I think this is a good compromise. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

@codecae Looks good to me! Thanks! 👍

mikeller
mikeller previously approved these changes Jun 8, 2020
Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

Looks good to me. Can I get you to rebase / squash please?

@codecae
Copy link
Member Author

codecae commented Jun 8, 2020

Will do 👍

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 8, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@mikeller mikeller merged commit f58c429 into betaflight:master Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants