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 the option to print system time rather than RTOS time in ESP_LOGx functions (IDFGH-1773) #3958

Closed
wants to merge 9 commits into from

Conversation

badcf00d
Copy link
Contributor

@badcf00d badcf00d commented Aug 23, 2019

I mainly wrote this because I wanted to syncronize logs from an ESP32 with logs from a cloud platform that were timestamped in UTC, and I think other people would probably find this option useful too.

Obviously this time will just start from 0 unless you set the system time (with an SNTP sync for example), but assuming you do set the time it makes the logs much more useful when trying to match up events happening on multiple platforms.

Example from the hello world program:

I (168) cpu_start: Chip Revision: 1
I (170) cpu_start: Starting scheduler on PRO CPU.
D (0) intr_alloc: Connected src 25 to int 2 (cpu 1)
I (0) cpu_start: Starting scheduler on APP CPU.
D (171) heap_init: New heap initialised at 0x3ffe0440
D (171) heap_init: New heap initialised at 0x3ffe4350
D (171) intr_alloc: Connected src 16 to int 12 (cpu 0)
Hello world!
D (00:00:00.011) efuse: coding scheme 0
D (00:00:00.012) efuse: In EFUSE_BLK0__DATA3_REG is used 1 bits starting with 15 bit
D (00:00:00.013) efuse: coding scheme 0
D (00:00:00.013) efuse: In EFUSE_BLK0__DATA5_REG is used 1 bits starting with 20 bit
This is ESP32 chip with 2 CPU cores, WiFi/BT/BLE, silicon revision 1, 4MB external flash
Restarting in 10 seconds...

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

Hi @Frosticles,

Thanks for contributing this, it looks like a very useful change for some users.

I have a couple of minor requests but overall looks great.

components/log/Kconfig Outdated Show resolved Hide resolved
components/log/include/esp_log.h Show resolved Hide resolved
components/log/log.c Outdated Show resolved Hide resolved
@projectgus projectgus requested a review from igrr August 29, 2019 04:03
@badcf00d
Copy link
Contributor Author

I've added in the requests from @projectgus, given it a quick test my end and all seems well. Please do comment on any other changes you'd like but otherwise I'm happy to merge this for now.

@github-actions github-actions bot changed the title Added the option to print system time rather than RTOS time in ESP_LOGx functions Added the option to print system time rather than RTOS time in ESP_LOGx functions (IDFGH-1773) Aug 30, 2019
@badcf00d badcf00d requested a review from projectgus September 8, 2019 19:19
Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

Thanks for the quick update @Frosticles and for being patient while I got back to you.

A couple of small things, one idea I didn't think of the first time around (sorry).

Also a very minor thing: the IDF code style is to put the opening brace of a statement on the same line as the statement, ie

 if ((timestamp > 0) || (i == 0)) {


if (bufferLock == 0)
{
_lock_init(&bufferLock);
Copy link
Contributor

@projectgus projectgus Sep 13, 2019

Choose a reason for hiding this comment

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

newlib locks actually lazy initialize, so you don't need this init check (the lock will be initialized the first time _lock_acquire() is called.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok, didn't know that, I'll remove this bit

components/log/log.c Show resolved Hide resolved
@projectgus
Copy link
Contributor

Thanks again @Frosticles . I've squashed the commits and pushed them into our internal review & merge queue. This PR will be updated again once that process is complete.

@badcf00d
Copy link
Contributor Author

badcf00d commented Oct 1, 2019

Apologies for the late commit, not sure how I missed that but cppcheck spotted that I was accessing that buffer array out of bounds, just pushed a fix.

@projectgus
Copy link
Contributor

@Frosticles Thanks, can't believe we both missed that! Have updated our internal branch, should be merged soon (has been a national holiday in China so things got a little delayed).

mahavirj pushed a commit to mahavirj/esp-idf that referenced this pull request Oct 17, 2019
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@projectgus
Copy link
Contributor

Oops, sorry this wasn't closed earlier @badcf00d . Changes were cherry-picked as
23e9224 .

@projectgus projectgus closed this Mar 15, 2020
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