[Linux] Add mechanism to redirect CHIP log other than stdout #33338
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andyg-apple,
anush-apple,
arkq,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
fessehaeve,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kiel-apple,
kkasperczyk-no,
kpschoedel,
ksperling-apple,
lazarkov and
lpbeliveau-silabs
pullapprove
bot
requested review from
rcasallas-silabs,
robszewczyk,
saurabhst,
selissia,
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang and
yunhanw-google
|
PR #33338: Size comparison from 242accd to 664b050 Increases (12 builds for linux)
Decreases (11 builds for efr32, linux)
Full report (77 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
yunhanw-google
approved these changes
May 7, 2024
| logFile = fopen(filePath, "a"); | ||
| if (logFile == nullptr) | ||
| { | ||
| perror("Failed to open log file"); |
There was a problem hiding this comment.
This must VerifyOrDie.
Comment on lines
+43
to
+50
| void OpenLogFile(const char * filePath); | ||
|
|
||
| /** | ||
| * Close the log file if it is open. This function should be called | ||
| * at the end of the application to ensure that all log messages are | ||
| * flushed to disk and the file is properly closed. | ||
| */ | ||
| void CloseLogFile(); |
There was a problem hiding this comment.
Where are these called?
| * exist, it will be created. If it exists, log | ||
| * messages will be appended to it. | ||
| */ | ||
| void OpenLogFile(const char * filePath); |
There was a problem hiding this comment.
We should have a list of log sinks (one of which is stdout by default) rather than assume a single place where logs go. Otherwise it is impossible to have stdout + output to file logs.
There was a problem hiding this comment.
We do currently allow setting one log sink (the logging redirector setup), which can then be used to multiplex as desired without changing any platform implementations....
There was a problem hiding this comment.
https://github.com/project-chip/connectedhomeip/blob/master/src/lib/support/logging/TextOnlyLogging.h#L359 contains a SetLogRedirectCallback where you can move your logging to go anywhere. This is what python uses to redirect to python logging instead of just console.
Could we use this for the CLI bits instead? One could even imagine some term-linke thing where you can both do cli-shell-interact and view the log in a separate pane...
There was a problem hiding this comment.
Yes, this method works.
tcarmelveilleux
requested changes
May 7, 2024
There was a problem hiding this comment.
- Introduce a log sink interface (base class) with LogV. Concrete implementations can have their own init, and are responsiblity of the owner/caller
- Make stdout a default log sink for linux
- Do not add new file methods to src/platform/logging --> not portable or relevant everywhere. AddLogSink/RemoveLogSink should be enough
- Ensure >1 log sink can be set, at least on Linux, so that when we update chip-tool interactive we have a way to not lose the logs, even on interactive, if needed.
There was a problem hiding this comment.
Why is this Linux-specific?
|
Close this PR since we already have the mechanism to redirect the logging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, both shell command and CHIP logs are printed out to the same console on Linux, make it very difficult to interact with the example app via shell CLI.
Event though I can turn off those logs, but it is better to have a way to redirect logging to another place so that I can only see CLI message in console and check CHIP log in another place