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

chore: Use Zephyr logging macros directly #121

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

vpodzime
Copy link
Collaborator

@vpodzime vpodzime commented Nov 22, 2024

Zephyr logging is a complex thing that does a lot of magic and optimizations. Using the macros from one place with a single '%s' argument leaves no room for magic. By using the macros directly from various places in our sources and with the various arguments, we give the magic a chance to happen.

Note

This makes zero difference in how things work (or rather doesn't) on my ESP32-EVB board. The only difference is that I now see the full URL:

[00:00:09.878,000] <inf> mender: Downloading artifact with id 'f47ef20...', name 'update56', uri 'https://c271964d41749feb10da762816c952ee.r2.cloudflarestorage.com/mender-artifacts-us/66eaa007a9b44e490044b0f5/d1758124-ab9d-4e8f-b05a-1d511d29826c?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=a848c3308d7d52663bbd129dff6a18b7%2F20241122%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20241122T123414Z&X-Amz-Expires=86400&X-Amz-SignedHeaders=host&response-content-disposition=attachment%3B%20filename%3D%22update56.mender%22&response-content-type=application%2Fvnd.mender-artifact&x-id=GetObject&X-Amz-Signature=04dfcf135a363040852b004034ad7cacb4745447d32bc7a3bc4b1a72ae41defc'

(so the changes are applied and "working")

Zephyr logging is a complex thing that does a lot of magic and
optimizations. Using the macros from one place with a single '%s'
argument leaves no room for magic. By using the macros directly
from various places in our sources and with the various
arguments, we give the magic a chance to happen.

Signed-off-by: Vratislav Podzimek <[email protected]>
@pasinskim
Copy link
Member

At least we know now. We must look somewhere else to remove crashes.

Copy link
Collaborator

@lluiscampos lluiscampos left a comment

Choose a reason for hiding this comment

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

I agree in that the logging is the biggest issue that we have right now when it comes to stack usage.

But with this change I don't see the vision of what mender-log is going to become. We are now mixing platform specific code in the header file (I understand why), but with that the implementation in the .c an the whole concept of this api is redundant.

We cannot do much right now with the current structure of public APIs. But this ideally should not be exposed as a library (mender-log.h) and just use a set of macros in mender-utils.h like MENDER_LOG_INFO that does LOG_INFO on Zephyr, and SOMETHING_ELSE in future platforms .

@vpodzime
Copy link
Collaborator Author

But this ideally should not be exposed as a library (mender-log.h) and just use a set of macros in mender-utils.h like MENDER_LOG_INFO that does LOG_INFO on Zephyr, and SOMETHING_ELSE in future platforms .

I don't see a difference between this being in mender-utils.h and in mender-log.h. Except for better readability and nicer structure in the latter case. Esp. if it has #ifdefs which make things harder to read. The fact that something is a separate header file doesn't mean it's a "library". I'd actually put the zephyr stuff into mender-log-zephyr.h and do

#ifdef __ZEPHYR__
#include "mender-log-zephyr.h"
#endif

in mender-log.h.

@lluiscampos
Copy link
Collaborator

lluiscampos commented Nov 26, 2024

But this ideally should not be exposed as a library (mender-log.h) and just use a set of macros in mender-utils.h like MENDER_LOG_INFO that does LOG_INFO on Zephyr, and SOMETHING_ELSE in future platforms .

I don't see a difference between this being in mender-utils.h and in mender-log.h.

My point is to drop the .c file(s). Hence making it header only once it is already platform specific in there.

I'd actually put the zephyr stuff into mender-log-zephyr.h and do (...)

Yes, and drop the .c files :) That would be ideal I think.

@vpodzime
Copy link
Collaborator Author

My point is to drop the .c file(s). Hence making it header only once it is already platform specific in there.

But you need to do some initialization. The mender_log_init() is empty for now, but still we at least need to do LOG_MODULE_REGISTER() exactly once somewhere. Why do you think having a .c file as well is an issue?

Copy link
Collaborator

@lluiscampos lluiscampos left a comment

Choose a reason for hiding this comment

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

My point is to drop the .c file(s). Hence making it header only once it is already platform specific in there.

But you need to do some initialization. The mender_log_init() is empty for now, but still we at least need to do LOG_MODULE_REGISTER() exactly once somewhere.

You are totally right.

Why do you think having a .c file as well is an issue?

Is just the whole concept of it as a portable library (not technically a library, but I don't know how to call it) is strange when the platform code is already in the header file.

Anyway, I think I took this discussion to far away, the core problem of this PR is solved, and the resulting binary code does what I think is best: map one to one into Zephyr log macros. Let's move into other problems 👍

@vpodzime vpodzime merged commit bd83876 into mendersoftware:main Nov 27, 2024
2 checks passed
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.

4 participants