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 option to use datetime stamp during logging. (IDFGH-3855) #5761

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Molorius
Copy link

Added option to use datetime stamp during logging.

Also changed log timestamp formatting to use unsigned int, allowing for smaller buffer.

Changed log timestamp formatting to use unsigned int, allowing for smaller buffer.
@CLAassistant
Copy link

CLAassistant commented Aug 20, 2020

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.

@github-actions github-actions bot changed the title Added option to use datetime stamp during logging. Added option to use datetime stamp during logging. (IDFGH-3855) Aug 20, 2020
@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution.

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 @Molorius ,

Thanks for sending this. I have a couple of minor comments but the change looks very useful to me, look forward to integrating it.

@@ -70,4 +70,12 @@ menu "Log output"
bool "System Time"
endchoice

config LOG_TIMESTAMP_SOURCE_SYSTEM_DATETIME
Copy link
Contributor

@projectgus projectgus Aug 26, 2020

Choose a reason for hiding this comment

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

Relatively minor, but for ease of use suggest adding this as a "choice" so the choices become "Milliseconds Since Boot", "System Time", "System Date and Time"

(Means some of the macro guards in esp_log.h will need to be updated as well.)

Copy link
Author

Choose a reason for hiding this comment

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

It is now another choice in the kconfig.

@@ -52,7 +52,11 @@ void esp_log_impl_unlock(void)

char *esp_log_system_timestamp(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

The description of this function in esp_log.h needs to be updated to match

Copy link
Author

Choose a reason for hiding this comment

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

Would you prefer changing the name of the function or splitting it into esp_log_system_timestamp and esp_log_system_datetimestamp? The xTaskGetSchedulerState portion would be put into a static function for use between both.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Molorius Good point! That is probably a cleaner solution, thank you for pointing it out.

However if it becomes less clean to implement in some way, I'm OK with the current implementation - it just needs to be noted in the header that it might return a date+time depending on config.

Copy link
Author

Choose a reason for hiding this comment

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

I separated the datetime elements into a separate function and updated esp_log.h to match.

Added better comments for datetimestamp function.
Have datetimestamp as a full choice in kconfig rather than an additional option.
@projectgus
Copy link
Contributor

Hi @Molorius ,

Thanks for being patient while I got back to you. This looks great to me, I've squashed the commits and pushed it into our internal review & merge queue. The PR will be updated once it has merged.

Angus

@projectgus
Copy link
Contributor

As it happens, at the last minute we've decided to try and implement this in a different way. Rather than having to add a whole new function and kconfig item for each new format, we're going to try to find an easier way to customize the log timestamp.

You don't need to do anything, this PR will still be updated once this functionality is available.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Apr 7, 2022
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Opened Issue is new labels Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants