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.

Sign up for GitHub

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

Jump to bottom

Remove excessive info and debug prints in SPIF driver #10501

Merged
merged 1 commit into from
Jun 3, 2019
Merged

Remove excessive info and debug prints in SPIF driver #10501

merged 1 commit into from
Jun 3, 2019

Conversation

davidsaada
Copy link
Contributor

Description

Remove excessive info and debug prints in SPIF driver.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@teetak01

@ciarmcom ciarmcom requested review from teetak01 and a team April 28, 2019 11:00
@ciarmcom
Copy link
Member

@davidsaada, thank you for your changes.
@teetak01 @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@offirko
Copy link
Contributor

offirko commented Apr 29, 2019

@davidsaada - I would personally keep most of those logs.
I believe that the trace log strategic use should be : ERROR+WARNING level for product (such as mbed-client) , Adding INFO for Development stages and DEBUG for debugging. Removing all of the debug and info logs kinda miss the point.. and will cripple any debug efforts that might occur in the future. Instead I would configure the mbed-client for trace log according to product life-cycle for each product.

Also, the thumb rule for me is keep INFO logs for critical stages that occur in low frequency (such as once in init). There are here a few INFO logs that are placed in higher frequency functions, such as erase, I would remove them and keep the rest.. but again its a question of trace logging strategy..

Thanks

@davidsaada
Copy link
Contributor Author

@davidsaada - I would personally keep most of those logs.
I believe that the trace log strategic use should be : ERROR+WARNING level for product (such as mbed-client) , Adding INFO for Development stages and DEBUG for debugging. Removing all of the debug and info logs kinda miss the point.. and will cripple any debug efforts that might occur in the future. Instead I would configure the mbed-client for trace log according to product life-cycle for each product.

Also, the thumb rule for me is keep INFO logs for critical stages that occur in low frequency (such as once in init). There are here a few INFO logs that are placed in higher frequency functions, such as erase, I would remove them and keep the rest.. but again its a question of trace logging strategy..

Thanks

Tend to agree, however this is way above the scope of this PR. Currently, all info and debug traces are enabled by default by client code, which is the trigger for the problem this PR answers.

@teetak01
Copy link
Contributor

I would suggest that all Mbed OS drivers would have mbed_lib.json configuration which allows enabling driver-internal tracing when requested. This would mean that traces like this could be kept in the source-code, but would require separate enablement.

For. ex. how ESP8266 is enabling AT-level debug traces:

https://github.com/ARMmbed/mbed-os/blob/master/components/wifi/esp8266-driver/mbed_lib.json#L24-L27

@offirko
Copy link
Contributor

offirko commented Apr 29, 2019

lo and behold.. mbed-os has exactly this framework :
https://github.com/ARMmbed/mbed-os/tree/7027eac9c6a6138e69a045391f4707e50597ecb9/features/frameworks/mbed-trace

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 29, 2019

I would suggest that all Mbed OS drivers would have mbed_lib.json configuration which allows enabling driver-internal tracing when requested. This would mean that traces like this could be kept in the source-code, but would require separate enablement.

instead of removing, rather disabling it in this PR. Or is there still an issue?

@bulislaw
Copy link
Member

The logs may be useful, but not when they are printed in all debug/develop builds. I'm very much for having the per-module-config in mbed_lib.json as suggested, disabled by default.

@0xc0170
Copy link
Contributor

0xc0170 commented May 15, 2019

@davidsaada please update

@davidsaada
Copy link
Contributor Author

Sorry for not getting back to your comments earlier. Been a bit busy.
@teetak01 @bulislaw @0xc0170
What you suggest is to use a local debug mechanism for debug instead of mbed_trace. Please note that this module (along with QSPIF, which I should resolve the same way) uses mbed_trace for errors and warnings as well. So, if I accept your suggestions, we'll get multiple logging mechanisms in the same module - mbed_trace for errors & warnings and a local debug mechanism for info & debug. I can go that way, but it seems very awkward to me.
@offirko
I totally agree with you regarding the way to go in the client. Debug messages should not be enabled by default in client. This misses the whole point. This would be the correct way to resolve this issue. However, it's outside the scope of this PR.

@davidsaada
Copy link
Contributor Author

Sorry, double checked with client folks. Apparently tr_debug is disabled by default there, while tr_info is enabled. The modified prints are all under tr_info, so will modify them to tr_debug.

@adbridge
Copy link
Contributor

@teetak01 @bulislaw @0xc0170 are you happy with the responses ?

@teetak01
Copy link
Contributor

What is going to be the final fix? Currently this PR is removing all those traces, so I am fine with that.

IMHO enabling those traces even on tr_debug() level is too much, as this is comparable of Mbed TLS enabling its debug-logs by default for all customers.

@0xc0170
Copy link
Contributor

0xc0170 commented May 31, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented May 31, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit 4f6035e into ARMmbed:master Jun 3, 2019
michalpasztamobica added a commit to michalpasztamobica/mbed-os that referenced this pull request Aug 5, 2019
@michalpasztamobica
Bring back module-specific debug logs
dbd47b6 
The logs are switched of by default and can be enabled with a
module-specific compile switch in mbed_app.json.

Brought back from PR ARMmbed#10501
@michalpasztamobica michalpasztamobica mentioned this pull request Aug 5, 2019
Merged
michalpasztamobica added a commit to michalpasztamobica/mbed-os that referenced this pull request Aug 5, 2019
@michalpasztamobica
Bring back SPIF module-specific debug logs
e96d782 
The logs are switched off by default and can be enabled with a
module-specific compile switch in mbed_app.json.

Brought back from PR ARMmbed#10501
michalpasztamobica added a commit to michalpasztamobica/mbed-os that referenced this pull request Aug 6, 2019
@michalpasztamobica
Bring back SPIF module-specific debug logs
a5bbf2d 
The logs are switched off by default and can be enabled with a
module-specific compile switch in mbed_app.json.

Logs brought back from PR ARMmbed#10501
michalpasztamobica added a commit to michalpasztamobica/mbed-os that referenced this pull request Aug 6, 2019
@michalpasztamobica
Bring back SPIF module-specific debug logs
1fe59b7 
The logs are switched off by default and can be enabled with a
module-specific compile switch in mbed_app.json.

Logs brought back from PR ARMmbed#10501
0xc0170 pushed a commit that referenced this pull request Aug 9, 2019
@michalpasztamobica @0xc0170
Bring back SPIF module-specific debug logs
ce65c8f 
The logs are switched off by default and can be enabled with a
module-specific compile switch in mbed_app.json.

Logs brought back from PR #10501
0xc0170 pushed a commit that referenced this pull request Aug 9, 2019
@michalpasztamobica @0xc0170
Bring back SPIF module-specific debug logs
e6908c5 
The logs are switched off by default and can be enabled with a
module-specific compile switch in mbed_app.json.

Logs brought back from PR #10501
0xc0170 pushed a commit that referenced this pull request Aug 9, 2019
@michalpasztamobica @0xc0170
Bring back SPIF module-specific debug logs
d9784cf 
The logs are switched off by default and can be enabled with a
module-specific compile switch in mbed_app.json.

Logs brought back from PR #10501
0xc0170 pushed a commit that referenced this pull request Aug 12, 2019
@michalpasztamobica @0xc0170
Bring back SPIF module-specific debug logs
dc55dae 
The logs are switched off by default and can be enabled with a
module-specific compile switch in mbed_app.json.

Logs brought back from PR #10501
Tharazi97 pushed a commit to Tharazi97/mbed-os that referenced this pull request Aug 14, 2019
@michalpasztamobica @Tharazi97
Bring back SPIF module-specific debug logs
4c0a333 
The logs are switched off by default and can be enabled with a
module-specific compile switch in mbed_app.json.

Logs brought back from PR ARMmbed#10501
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@offirko offirko offirko approved these changes

@AriParkkila AriParkkila AriParkkila approved these changes

@teetak01 teetak01 teetak01 approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

Assignees
No one assigned
Labels
release-version: 5.13.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@davidsaada @ciarmcom @offirko @teetak01 @0xc0170 @bulislaw @adbridge @mbed-ci @AriParkkila