-
Notifications
You must be signed in to change notification settings - Fork 162
ui: sensors page #739
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
ui: sensors page #739
Conversation
@@ -75,6 +75,7 @@ class HomeScreen : public UIScreen { | |||
RADIO, | |||
BLUETOOTH, | |||
ADVERT, | |||
SENSORS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am just wondering what % of users will use this, and if the majority will just always be having to button press over this to get to the menu they want(?) I have no idea. But only consideration is maybe a conditional compile of this feature(?) But, then we have the problem if mushrooming build variants for releases :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, then we have the problem if mushrooming build variants for releases :-(
I guess this problem will occur anyway, sooner or later. For devices with low screen size and low flash size at some point adding any additional feature will cause limitations for users that don't actually need that feature or use hardware that doesn't provide the necessary resources.
That's one reason for me to come up with this proposal. It won't prevent the mushrooming of build configurations in general, but it provides a way to handle them reasonably.
@@ -113,9 +114,32 @@ class HomeScreen : public UIScreen { | |||
display.fillRect(iconX + 2, iconY + 2, fillWidth, iconHeight - 4); | |||
} | |||
|
|||
DynamicJsonDocument _sensors_doc; | |||
JsonArray _sensors_arr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been resisting adding this lib as dependency. It's pretty HUGE, and I have a lighter-weright LPPReader class that is a simpler alternative.
int next_sensors_refresh = 0; | ||
|
||
void refresh_sensors() { | ||
CayenneLPP lpp(200); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make this a member var, as it involves a malloc()/free() under the hood. Is best to minimise these.
In last push, I've stopped using ArduinoJSON in favour of Scott's LPPHelpers, which are lighter (and easier to use) Sensors page is included conditionally (based on UI_SENSORS_PAGE define), set to 1 on TEcho I decided to only put the rendering (and event) code in conditionals, there are too many #if / #endifs otherwise ... |
We could also hide the sensors page if there is no sensors on the node ? It would need some modification to have a more dynamic list of page (I thought about a table containing the identifiers of displayed pages) |
synthetic display of sensors value, scrolls if not enough lines