[Quality of Developer Life] Add a new telnet/serial debugger command: uptime.#402
Conversation
… uptime. I've had this in my tree for a while, but while soaking the new effects, it's actually justified its bits per blinken, IMO. Help text: (processRemoteDebugCmd)(C1) uptime Show system uptime, reset reason Sample invocation: uptime * Debug: Command received: uptime (I) (ShowUptime)(C1) Uptime: 0 days - 00:04:50 (I) (ShowUptime)(C1) Last boot reason: Power On (I) (loop)(C1) Here (I) (esp32_partition_table_read)(C1) Reading partitions (I) (esp32_partition_table_read)(C1) nvs 36864 8192 It's quite ESP32-specific in some ways and it's definitely force-fit into ntpclient (hey, it's gotta be somewhere, right?) but I really don't think it's any weirder or less justified that a lot of the existing code. I had a version of this that displayed the uptime in an overlay, but it destroyed the FPS. If someone is interested in pair-programming a follow-up to this PR that adds that, I'm down.
|
I suppose I should disclose that I've never really leaned in on the cases about leap seconds, ntp adjustments behind our backs, calendar wraparound, or even really anything longer than 24 hours. (Even for small values of "24".) I care if it ran all night long without resetting. Sure, I could set up SmokePing or Mosquitto or something to track this. This was easier and it served my needs. "Not all things worth doing are worth doing well." It is not "web scale". Well, by current twitter standards, maybe it is... |
rbergen
left a comment
rbergen
left a comment
There was a problem hiding this comment.
Nice, I like it!
@robertlipe I have a few thoughts about the comments, but certainly nothing substantial enough to block a merge. Let me know what you think; again, I'm happy to approve regardless.
|
Mostly agreed. Update forthcoming later.
…On Fri, Aug 11, 2023 at 4:46 AM Rutger van Bergen ***@***.***> wrote:
***@***.**** commented on this pull request.
Nice, I like it!
@robertlipe <https://github.com/robertlipe> I have a few thoughts about
the comments, but certainly nothing substantial enough to block a merge.
Let me know what you think; again, I'm happy to approve regardless.
------------------------------
In src/ntptimeclient.cpp
<#402 (comment)>
:
> +// This is underdesigned for general use. It's very specifically a
+// throwaway meant only for debugging. It's not really NTP related.
+//
+// It contains some ESP32-specific goo, but that's allowed elsewhere.
I don't think you need to label this as a throwaway. We've recently
concluded that this is a developer-oriented project "with one exception",
so a developer-oriented feature is a feature.
I'm also not sure it's necessary to include "defensive ponderings" about
implementation decisions in the source code comments. Where appropriate,
comments about implementation decisions can be added to the PR when it's
opened.
(I'm aware there may be other places in the code where these comments
exist, but up to this point they've been grandfathered as artifacts from
before the project was made publicly available.)
------------------------------
In src/ntptimeclient.cpp
<#402 (comment)>
:
> + // I hate C99 time handling so much, but C17's <chrono> isn't likely
+ // worth the lift.
See second part of previous comment.
------------------------------
In src/ntptimeclient.cpp
<#402 (comment)>
:
> +
+ const char* reason_text = "Unknown";
+ esp_reset_reason_t reason = esp_reset_reason();
+ switch (reason)
+ {
+ case ESP_RST_POWERON: reason_text = "Power On"; break;
+ case ESP_RST_EXT: reason_text = "External Pin"; break;
+ case ESP_RST_SW: reason_text = "Software Restart"; break;
+ case ESP_RST_PANIC: reason_text = "Panic"; break;
+ case ESP_RST_INT_WDT: reason_text = "Watchdog barked"; break;
+ case ESP_RST_TASK_WDT: reason_text = "Task Watchdog barked"; break;
+ case ESP_RST_WDT: reason_text = "Other Watchdog barked"; break;
+ case ESP_RST_DEEPSLEEP: reason_text = "Reset in deep sleep"; break;
+ case ESP_RST_BROWNOUT: reason_text = "Brownout"; break;
+ case ESP_RST_SDIO: reason_text = "Reset over SDIO"; break;
+ // Documented, but seemingly not implemented yet.
That's interesting! How did you establish it's not implemented yet?
While writing this, the question comes up if it would make sense to still
include the case anyway? If it's not implemented it won't break anything,
and this will "magically" start working if and when it is.
—
Reply to this email directly, view it on GitHub
<#402 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD377IM4UDLVXOIC3JO3XUX5ODANCNFSM6AAAAAA3JXLU4Y>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
|
Guidance on embedding design considerations noted. Thank you. See if this one is mellow enough for a commit. It builds everywhere. It does leak symbols into !ENABLE_WIFI and !INCOMING_WIFI_ENABLED builds, but so does the rest of that file. {shrug} -fsymbols-sections should harvest the space. Probably. "Not all things worth doing are worth doing well." |
|
CI fails because the PlatformIO Core installer script, as provided by PlatformIO, seems to have gone missing. I opened platformio/platformio-core#4715 to report this. |
You don't need to use the PlatformIO Core installer in the CI instances. See docs https://docs.platformio.org/en/latest/integration/ci/github-actions.html Just use |
@ivankravets Thank you for commenting. I'm aware PlatformIO Core can also be installed using pip, which is actually what we did in the past. How I read the PlatformIO Core documentation is that it clearly labels the installer script installation method as "generally recommended", though. About the installer script documentation page, there is another issue with that. The one-liner (" |
rbergen
left a comment
rbergen
left a comment
There was a problem hiding this comment.
@robertlipe Thanks for your changes. LGTM and with CI back in working order, I'll proceed to merge.
|
Thank you, Sir.
…On Mon, Aug 14, 2023 at 3:34 AM Rutger van Bergen ***@***.***> wrote:
Merged #402
<#402> into
main.
—
Reply to this email directly, view it on GitHub
<#402 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD372EYWRYEESTT4H52LXVHPJXANCNFSM6AAAAAA3JXLU4Y>
.
You are receiving this because you were mentioned.Message ID:
<PlummersSoftwareLLC/NightDriverStrip/pull/402/issue_event/10083787747@
github.com>
|
|
@rbergen, thanks! I've just updated the docs platformio/platformio-docs@6b2af96...eca1ad5#diff-7472577066642c8cf999095c9b575e362a6315d0d69c50685dd175752fc5f42b The installer script is recommended for developers on personal machines where Nevertheless, the CI machines are stateless. So, using |
|
@ivankravets Great, that's a very quick turnaround time! :) Thanks for your explanation, I understand the distinction between CI and local. After bumping our noses against a number of issues related to differences between environments, we try to keep CI as close to local as possible. So, as long as the installer script is the recommended approach for local, then we'll also use that for CI. |
3 participants
I've had this in my tree for a while, but while soaking the new effects,
it's actually justified its bits per blinken, IMO.
Help text:
(processRemoteDebugCmd)(C1) uptime Show system uptime, reset reason
Sample invocation:
uptime
(I) (ShowUptime)(C1) Uptime: 0 days - 00:04:50
(I) (ShowUptime)(C1) Last boot reason: Power On
(I) (loop)(C1) Here
(I) (esp32_partition_table_read)(C1) Reading partitions
(I) (esp32_partition_table_read)(C1) nvs 36864 8192
It's quite ESP32-specific in some ways and it's definitely force-fit
into ntpclient (hey, it's gotta be somewhere, right?) but I really don't
think it's any weirder or less justified that a lot of the existing
code.
I had a version of this that displayed the uptime in an overlay, but it
destroyed the FPS. If someone is interested in pair-programming a
follow-up to this PR that adds that, I'm down.
Description
Contributing requirements
mainas the target branch.