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

User API for logging #998

Merged
merged 7 commits into from
Jun 10, 2016
Merged

User API for logging #998

merged 7 commits into from
Jun 10, 2016

Conversation

sergeuz
Copy link
Member

@sergeuz sergeuz commented May 8, 2016

This PR adds initial version of the logging API for application-side code.

Basic usage:

#include "application.h"

// Enable logging to primary USB serial
SerialLogHandler logHandler(9600, LOG_LEVEL_ERROR, { // Log only errors by default
    { "app", LOG_LEVEL_ALL } // Log all application messages
});

// Global logger
Logger logger;

void setup() {
    logger.log("My device ID: %s", System.deviceID().c_str()); // Uses default logging level (INFO)

    logger.info("This is info message");
    logger.error("This is error message");

    if (logger.isTraceEnabled()) {
        for (int i = 0; i < 5; ++i) {
            logger.trace("This is trace message");
        }
    }
}

void loop() {
}

Serial output:

0000000044 app: INFO: My device ID: 1b002e000e47343432313031
0000000044 app: INFO: This is info message
0000000087 app: ERROR: This is error message
0000000135 app: TRACE: This is trace message
0000000183 app: TRACE: This is trace message
0000000231 app: TRACE: This is trace message
0000000279 app: TRACE: This is trace message
0000000327 app: TRACE: This is trace message

Advanced usage:

#include "application.h"

SerialLogHandler logHandler(9600, LOG_LEVEL_ERROR, {
    { "app", LOG_LEVEL_INFO },
    { "app.func", LOG_LEVEL_ALL },
    { "app.clazz", LOG_LEVEL_ALL }
});

void func() {
    static Logger log("app.func");
    log.trace("Hello from func()");
}

class Clazz {
public:
    Clazz() {
        log.trace("Hello from Clazz"); // Uses class-local logger
    }

private:
    static Logger log;
};

// Class-local logger
Logger Clazz::log("app.clazz");

// Global logger (uses "app" category by default)
Logger log;

void setup() {
    log.info("Hello from setup()"); // Uses global logger
    func();
    Clazz();
}

void loop() {
}

Serial output:

0000000071 app: INFO: Hello from setup()
0000000071 app.func: TRACE: Hello from func()
0000000117 app.clazz: TRACE: Hello from Clazz

Doneness:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • API tests compiled
  • Run unit/integration/application tests on device
  • Add documentation
  • Add to CHANGELOG.md after merging (add links to docs and issues)

@sergeuz
Copy link
Member Author

sergeuz commented May 8, 2016

Sorry for a bit noisy diff – I had to squash multiple commits while transiting from my forked repo. Key changes can be found here: https://github.com/spark/firmware/blob/feature/logger_api/wiring/inc/spark_wiring_logging.h#L173

@sergeuz
Copy link
Member Author

sergeuz commented May 8, 2016

Couple of questions to discuss:

Should we declare global Logger instance in one of our header files to make it accessible by default (like we currently do for Serial)?

Do we actually want to expose methods for direct logging, i.e. Logger::printf(), Logger::write()? Those can probably be confusing.

@monkbroc
Copy link
Member

The API looks good.

Should we declare global Logger instance in one of our header files to make it accessible by default (like we currently do for Serial)?

I'd mirror EEPROM: Create a singleton on first access.

Do we actually want to expose methods for direct logging, i.e. Logger::printf(), Logger::write()? Those can probably be confusing.

I don't think there's a need to expose printf and write.

I would like to see DUMP exposed since this is a common thing I want to do: check if a buffer has the values I expect during program run.

@m-mcgowan
Copy link
Contributor

This is a great start! 👍 For simpler programming, would using non-static loggers be a workable alternative? The main upside is that they can be declared and initialized within the class declaration.
Is there any consequence to having multiple logger instances for the same category?

Similarly for function loggers, how about:

void function()
{
    FunctionLogger log("app.function");
    // would be nice to use __FUNCTION_NAME__ here but the underscores are a bit scary! 
} 

@sergeuz sergeuz force-pushed the feature/logger_api branch from 523932d to 5391b74 Compare June 5, 2016 13:37
@sergeuz sergeuz force-pushed the feature/logger_api branch from 5391b74 to 9d6d998 Compare June 9, 2016 01:05
@sergeuz
Copy link
Member Author

sergeuz commented Jun 9, 2016

This should be ready for the review. Primary changes are:

  • Added default global logger (Log):
SerialLogHandler logHandler;

void setup() {
    Log.info("Hello!");
}
  • Reworked the documentation.
  • Moved LogManager to the public namespace.
  • Made baud rate argument optional for the existent log handlers.

@m-mcgowan m-mcgowan self-assigned this Jun 10, 2016
@m-mcgowan m-mcgowan added this to the 0.6.x milestone Jun 10, 2016
@m-mcgowan
Copy link
Contributor

@sergeuz - do you feel the API is stable enough that we should include it in 0.6.0, or is it best to postpone until 0.7.0?

@sergeuz
Copy link
Member Author

sergeuz commented Jun 10, 2016

@m-mcgowan I think it's stable and the code is quite straightforward to review. The documentation may require some editing though. If we don't have enough time to review both this PR and #1022 which depends on this branch and introduces ABI breaking changes, then I would postpone both PRs until 0.7.0.

@m-mcgowan m-mcgowan merged commit 05d6ceb into develop Jun 10, 2016
@m-mcgowan
Copy link
Contributor

Fantastic job! 👍

@technobly technobly deleted the feature/logger_api branch October 27, 2016 17:26
technobly added a commit that referenced this pull request Dec 22, 2016
a892020 unifying Since x.x.x firmware version messages
b153672 Since 0.7.0 -> Since 0.6.1
52a6313 Merge commit '60e0e09d54c2b5c1ebe875861da77a306d3c785b' into prerelease-dup
9dac290 More typos fixed
5295a53 corrected grammar (#524)
9f4b24b Spell check docs
fcca5c6 occaisonal --> occasional
60e0e09 Reference documentation
ef0dad7 Merge pull request #1190 from spark/feature/setup-button-mirror
d2eb670 Merge pull request #1195 from spark/fix/revert_publish_api
05d0200 Merge branch 'develop' into feature/listen-timeout
2b43c71 grammatical error
878ba19 Revert Particle.publish() API changes; docs for WITH_ACK flag
b7030c5 Update firmware.md
d84a942 Add advanced process control
fa03d96 Fix broken link on firmware page
03b8a78 Document Process::run
08047a0 EEPROM is borken on RPi
566a869 buttonMirror() documentation
ea100c7 Support for `Serial1` is not complete
468b33d Raspberry Pi I2C firmware docs
32e5ba4 Remove software timers from RPi docs.
26d7251 Merge branch 'develop' into feature/conn_state_events
e15b410 added v0.6.0 firmware notes
38614db adds Docs for set|getListenTimeout()
9949e2d adds usb-serial1 device tags
eee29b2 cleanup serial2 and manual edits from last merge with conflicts
616330f Merge branch 'master' into prerelease
4590339 Update firmware docs to use device features
8e1149f Add device feature flags
62312f0 Add Raspberry Pi to firmware, tools and guides
7168402 Update firmware.md
c93d248 Clarification of retained SRAM size
73f2e00 corrected typo 'sequencially' to 'sequentially'
648234c Merge pull request #468 from spark/feature/usb-docs-0.6.0
d3e5eba Reference docs
7794203 Merge branch 'master' into prerelease
edcf989 adds link to freertos4core particle library
1ffdd80 time_changed system event
dbc5d03 Time.isValid(), Particle.syncTimeDone()/syncTimePending()/timeSyncedLast() documentation
c380dfd [optional] needs to include the optional comma too
6f5c96d Merge branch 'master' into prerelease
6cf14d1 Update firmware.md
6e6e216 Merge branch 'master' into prerelease
f2cf630 adding notes for 0.5.1 and 0.5.2 firmware versions
6249120 adds reference.hbs back to firmware.md
6fa5464 ensure firmwareversions.js only loads on firmware.md aka firmware reference
4fc16b8 added CLI version, electron part 2/3 options and downgrading instructions
ad530eb Separate Firmware Release and Formatting Tweaks
aafbabe Create possibility for passing variables thorugh query params, for Firmware Releases
25cab35 Fixes typo
1aa4c18 Add B0-B5, C0-C5 to GPIO list in note for electron
c857e8a Merge branch 'master' into prerelease
dac4690 Keyboard and Mouse documentation
8d974bf Updated Serial documentation
fd24a4d Merge pull request #467 from dougalcampbell/patch-1
4d75a59 Missing doc for TCPClient(buffer, len)
1b667e5 Use `hostname` instead of `URL` terminology
d0e5903 Merge commit '568ceade3e1481cdb00dcdf51a97a737750a96bb' from firmware-docs into staging
d49ac39 Merge branch 'master' into feature/tutorials
e205803 Makes tests pass, fixing broken dashboard links
66a1955 Fix your product id link
81f7eb6 More Dashboard -> Console
266e459 fixed: markdown for threading docs
4f615c9 missing markdown for interrupts example
568cead Daylight Saving Time (DST) documentation
a696d8a new analogRead() pinMode behavior for 0.5.3 system firmware
aa2994a Fix description of sleep(pin, edge)
5c049dc fix typos
b69e421 Typo in SoftAP example
be62c64 Clarify behavior of 0 seconds in System.sleep() (#425)
838d6c7 missing parameters for printlnf()
7503310 relocated note
62acc8d missed # in condition 😊
f50a589 Update firmware.md
7bc380b Note WKP for deep sleep
af77e5e Merge branch 'feature/reset-network-flag' into develop
405e9e9 Documentation fixes
0415ab4 API docs
0e0a691 Minor fixes
53f993b API docs for logging attributes
18483a2 Merge pull request #991 from spark/feature/dac-pwm-resolution
747f8d0 Merge pull request #997 from spark/feature/serial-config
26eaba8 Merge pull request #998 from spark/feature/logger_api
b7f3081 Update flush() documentation
af32b6b Store last reset reason and publish it to the cloud (#944)
08d2d42 USART configuration documentation
20ebf60 API docs
29e91a1 reduce datausage on reset/wakeup (#953)
6665b03 typo
4caa81c One carriage return isn't enough to make a new paragraph (#414)
a4f05d6 Fix typo in handlebar if
c790bcb Take out factory reset for anything but the Core
d41c053 WIFI_CONNECT_NO_LISTEN actually is WIFI_CONNECT_SKIP_LISTEN
4db5752 Increase documented number of cloud functions and variables (#397)
80ff0e9 Add PRODUCT_ID to the firmware docs
dc99c6e Reword cloud function #389
66f4acf analogWriteResolution documentation
3602095 Fix layout in Serial.begin()
df96c32 Clarify include header for Serial2
761f7c4 explicitly show the particle.function protype
6667d16 add a single letter, whenver->whenever
79ea057 Wire.stretchClock() feature enabled by default for I2C Slave mode
f3ad8b5 made firmware.md a symlnk on linux...didn't take on OS X

git-subtree-dir: docs/reference
git-subtree-split: a892020dc28f10d348c813f63e77099993ecf5f6
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.

3 participants