Added option for sparkline#7
Conversation
Juerd
left a comment
Juerd
left a comment
There was a problem hiding this comment.
Thanks for the PR; a sparkline would be a nice configurable option. I haven't tried it yet and won't accept the PR in its current form, but have provided some remarks.
| @@ -0,0 +1,150 @@ | |||
| // Adaped version of ESParklines to sidestep random() issue in FixedPointsArduino library | |||
There was a problem hiding this comment.
I strongly prefer not including forked libraries in the Operame repository, if it can be avoided at all. I've submitted a PR to fix the underlying library instead, at Pharap/FixedPointsArduino#71
| bool wifi_enabled; | ||
| bool mqtt_enabled; | ||
| int max_failures; | ||
| bool sparkline_enable; |
There was a problem hiding this comment.
Nitpick: the other variables are called _enabled. For consistency, _enabled would be preferred to _enable.
| bool mqtt_enabled; | ||
| int max_failures; | ||
| bool sparkline_enable; | ||
| int sparkline_bufferlenght; |
| } | ||
| } | ||
|
|
||
| SparkLine<uint16_t> display_sparkline(766, [&](const uint16_t x0, const uint16_t y0, const uint16_t x1, const uint16_t y1) { |
There was a problem hiding this comment.
I'm guessing the 766 was supposed to be sparkline_bufferlength? That variable is declared and assigned but not used elsewhere.
| sprite.drawString(text, display.width()/2, display.height()/2); | ||
|
|
||
| if (sparkline_enable) { | ||
| display_sparkline.draw( 10, display.height()/4*3, display.width()-20, display.height()/2-5); |
There was a problem hiding this comment.
I'm curious what the -5 is for.
| delay(30); | ||
| } | ||
| display_logo(); | ||
| if (sparkline_enable ) display_sparkline.reset(); |
There was a problem hiding this comment.
I'm wondering if having the sparkline in the demo is worth resetting the main history for. OTOH, this makes it consistent with accessing the portal (resetting the microcontroller is the only way out) and does make the demo include the sparkline.
| mqtt_template = WiFiSettings.string("operame_mqtt_template", "{} PPM", T.config_mqtt_template); | ||
| WiFiSettings.info(T.config_template_info); | ||
|
|
||
| WiFiSettings.heading("sparkline"); |
|
|
||
| WiFiSettings.heading("sparkline"); | ||
| sparkline_enable = WiFiSettings.checkbox("operame_sparkline", false, T.config_sparkline); | ||
| sparkline_bufferlenght = WiFiSettings.integer("operame_sparkline_buffer", 0, 16383, 512, T.config_sparkline_buffer); ; |
There was a problem hiding this comment.
Maybe the buffer length should not be user configurable, at least not without explaining what it does. If it is configurable, I think it would be slightly more user friendly to let them specify the buffer size in minutes.
Why 16383 and 512?
3 participants
Added option for a sparkline.