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

logcfg: Allow logs to be defined in syscfg.yml #221

Closed
wants to merge 3 commits into from

Conversation

ccollins476ad
Copy link
Contributor

@ccollins476ad ccollins476ad commented Oct 16, 2018

This PR adds a new feature to newt: A package can define logs in its syscfg.yml file under the heading of syscfg.logs. During the build process, newt generates a C header file called logcfg/logcfg.h containing macros for using each generated log. This feature is described in more detail in the commit comments.

Please feel free to critique the feature itself in addition to its implementation.

This feature serves two purposes:

  1. Allow finer-grained control of which log messages to include in the build.
  2. Provide a means of visualizing the various log modules in the system.

Allow finer-grained control of which log messages to include in the build

Currently, the user can control which log messages get included in the build by setting the LOG_LEVEL syscfg setting. However, this setting applies to every log in the system, and may be too coarse to be useful. This PR allows the log level to be set per module rather than system-wide.

Provide a means of visualizing the various log modules in the system

Module IDs throughout the system must be unique. Since third-party packages may define their own log modules, it is currently quite difficult to enforce this constraint. This PR introduces a few newt commands (target log show and target log brief) that display tables describing the log modules in use by the system. Moreover, newt now aborts the build if it detects a log module conflict.

NOTE: There is a small sibling PR for mynewt-core: apache/mynewt-core#1461

A setting can refer to another with the `MYNEWT_VAL(...)` notation.
This commit extracts the code which detects such references and puts it
into separate functions.
@andrzej-kaczmarek
Copy link
Contributor

This is a really nice idea, hopefully someone can review Go code and we can merge this soon :-)

Just few things to consider:

  • what about some "don't care" (-1?) module id which means "first available value" (from some predefined range)?
  • this could also generate file with id -> name mappings so these can be returned by e.g. newtmgr and perhaps also used for some pretty-printing later
  • I'm not sure about the guest field. perhaps if we want to reuse id of some other module we could have some other field to reference this by name? my concern is that if we change module id of "host" module then it needs to be updated by all "guests"... if we reference by name, this is not a problem.

and I guess eventually we'll be able to remove hardcoded values from log_common.h and replace with this new feature?

@ccollins476ad
Copy link
Contributor Author

ccollins476ad commented Oct 25, 2018

Just few things to consider:

  • what about some "don't care" (-1?) module id which means "first available value" (from some predefined range)?

Good idea.

EDIT: On second thought, this might be problematic. This could cause module IDs to change when new packages are added to the target. Since each log entry contains its module ID, the result would be inconsistent log data.

  • this could also generate file with id -> name mappings so these can be returned by e.g. newtmgr and perhaps also used for some pretty-printing later

I think this is good also. I was actually thinking something similar after I submitted this PR. This would remove the need to manually register module names in the application.

  • I'm not sure about the guest field.

I am not sure about guest either. I don't remember what I was thinking exactly. I think there may be the desire for two different packages to log to the same module, but both packages need their own macros so that they can be disabled independently at build time. This may not be a real case after all. I am fine with removing guest to simplify the feature. We can re-add it later if necessary.

perhaps if we want to reuse id of some other module we could have some other field to reference this by name? my concern is that if we change module id of "host" module then it needs to be updated by all "guests"... if we reference by name, this is not a problem.

We can already reference a module by name if there is a dedicated syscfg setting (e.g., PKG1_LOG_MODULE). If PKG2 wants to use PKG1's log module, it can just specify MYNEWT_VAL(PKG1_LOG_MODULE) as its module ID. This requires a bit of extra work; each package needs to define syscfg settings for its module IDs, but I think this is desirable, as that lets the target override all module IDs in the system.

and I guess eventually we'll be able to remove hardcoded values from log_common.h and replace with this new feature?

Yes, I think that will be a good thing. We can also remove the distinction between "system modules" and "user modules" that exists today (system module IDs are in the range 0-63).

A package can define logs in its `syscfg.yml` file under the heading of
`syscfg.logs`.  During the build process, newt generates a C header file
called `logcfg/logcfg.h` containing macros for using each generated log.

Each entry in the `syscfg.logs` map has the following structure:

        <log-name>:
            module: <module>
            level: <level>
            guest: <true/false> (optional)

For example:

    syscfg.logs:
        MY_LOG:
            module: MYNEWT_VAL(MY_LOG_MODULE)
            level: MYNEWT_VAL(MY_LOG_LEVEL)

It is recommended, though not required, that the module and level fields
refer to syscfg settings, as above.  This allows the target to
reconfigure a package's log without modifying the package itself.

The above log definition generates the following code in
`logcfg/logcfg.h` (assuming `MY_LOG_MODULE is set to LOG_LEVEL_ERROR (3)):

    #define MY_LOG_DEBUG(logcfg_lvl_, ...) IGNORE(__VA_ARGS__)
    #define MY_LOG_INFO(logcfg_lvl_, ...) IGNORE(__VA_ARGS__)
    #define MY_LOG_WARN(logcfg_lvl_, ...) IGNORE(__VA_ARGS__)
    #define MY_LOG_ERROR(logcfg_lvl_, ...) MODLOG_ ## logcfg_lvl_(MYNEWT_VAL(MY_LOG_MODULE), __VA_ARGS__)
    #define MY_LOG_CRITICAL(logcfg_lvl_, ...) MODLOG_ ## logcfg_lvl_(MYNEWT_VAL(MY_LOG_MODULE), __VA_ARGS__)

If two or more logs have module values that resolve to the same number,
newt aborts the build with an error:

    Error: Log module conflicts detected:
        Module=100 Log=MY_LOG Package=sys/coredump
        Module=100 Log=YOUR_LOG Package=sys/coredump

    Resolve the problem by assigning unique module IDs to each log,
    or by setting the "guest" flag of all but one.

The "guest" flag, when set, allows a log to use the same module as
another without generating an error.
Add two new newt commands:

    newt target logcfg show <target>
    newt target logcfg brief <target>

Both commands produce a report of all the logs configured for the
target.  Example output:

[SHOW]
    $ newt target logcfg show slinky-nrf52dk

    Log config for targets/slinky-nrf52dk:
    COREDUMP_LOG:
        Package: sys/coredump
        Module:  100              [COREDUMP_LOG_MODULE]
        Level:   3 (ERROR)        [COREDUMP_LOG_LEVEL]
        Flags:

    SENSORS_LOG:
        Package: sys/coredump
        Module:  99
        Level:   1 (INFO)         [COREDUMP_LOG_LEVEL]
        Flags:

[BRIEF]
    $ newt target logcfg brief slinky-nrf52dk

    Brief log config for targets/slinky-nrf52dk:
                 LOG | MODULE   | LEVEL        | FLAGS
    -----------------+----------+--------------|----------
        COREDUMP_LOG | 100      | 3 (ERROR)    |
         SENSORS_LOG | 99       | 1 (INFO)     |
@ccollins476ad
Copy link
Contributor Author

By the way - I have removed the guest flag. I agree it is not very helpful.

@ccollins476ad
Copy link
Contributor Author

Closing; this was superseded by #230.

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.

None yet

3 participants