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

Diagnostics service #1390

Merged
merged 20 commits into from Oct 3, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
919a770
Device diagnostics: Initial data source API
sergeuz Sep 13, 2017
defe19c
Implemented data source API
sergeuz Sep 15, 2017
f4f445e
Enum and non-atomic classes; minor API changes
sergeuz Sep 17, 2017
89734c2
Unit tests; minor fixes
sergeuz Sep 17, 2017
58acf8a
Moved the locking functionality to policy classes
sergeuz Sep 18, 2017
1b90b54
adds a new describe message type for system metrics, and a callback t…
m-mcgowan Sep 18, 2017
ca4a396
enable the newer protocol implementation for TCP devices except the Core
m-mcgowan Sep 18, 2017
8141455
Revert "enable the newer protocol implementation for TCP devices exce…
m-mcgowan Sep 19, 2017
4de2446
Makes spark_wiring_diagnostics header only
avtolstoy Sep 19, 2017
c4ce53a
Adds __dso_handle to Electron system-part1 and system-part2 newlib stubs
avtolstoy Sep 19, 2017
ee742dd
Add stubs for CRT functions dealing with memory allocations to the bo…
sergeuz Sep 19, 2017
ad996b9
Use std::lock_guard with LockingPolicy classes; minor improvements in…
sergeuz Sep 19, 2017
77ae8ab
Use separate enums for data source and service commands; minor renamings
sergeuz Sep 19, 2017
7cfe614
Use uint16_t for structure size fields
sergeuz Sep 19, 2017
015b360
Add return value to the enumeration callback
sergeuz Sep 20, 2017
b101c09
Add a system function to dump the diagnostic data in JSON format
sergeuz Sep 20, 2017
afca0d7
Define system data source IDs
sergeuz Sep 21, 2017
75e8264
Fix trailing commas in generated JSON data
sergeuz Sep 21, 2017
555c631
Add 'd' command to print the diagnostic data to the setup console
sergeuz Sep 22, 2017
bcc4986
Allocate the service's singleton with 'new' to save some bytes in part1
sergeuz Sep 26, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions modules/photon/system-part1/src/newlib.cpp
@@ -1 +1,4 @@
#include "../../../../hal/src/stm32/newlib.cpp"

// Defining this in hal/src/stm32/newlib.cpp causes linker errors
void *__dso_handle = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know what has changed to make this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

part1 now has a global variable (Diagnostics) with a non-trivial destructor

72 changes: 72 additions & 0 deletions services/inc/diagnostics.h
@@ -0,0 +1,72 @@
/*
* Copyright (c) 2017 Particle Industries, Inc. All rights reserved.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation, either
* version 3 of the License, or (at your option) any later version.
*
* This library is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this library; if not, see <http://www.gnu.org/licenses/>.
*/

#pragma once

#include <stdint.h>
#include <stddef.h>

// System data sources
#define DIAG_SOURCE_INVALID 0 // Invalid source ID
#define DIAG_SOURCE_USER 1024 // Base value for application-specific source IDs

#ifdef __cplusplus
extern "C" {
#endif

// Data types
typedef enum diag_type {
DIAG_TYPE_INT = 1 // 32-bit integer
} diag_type;

// Data source and service commands
typedef enum diag_cmd {
DIAG_CMD_RESET = 1,
DIAG_CMD_ENABLE = 2,
DIAG_CMD_DISABLE = 3,
DIAG_CMD_GET = 4
} diag_cmd;

typedef struct diag_source diag_source;

typedef int(*diag_source_cmd_callback)(const diag_source* src, int cmd, void* data);
typedef void(*diag_enum_sources_callback)(const diag_source* src, void* data);

typedef struct diag_source {
size_t size; // Size of this structure
Copy link
Contributor

@m-mcgowan m-mcgowan Sep 18, 2017

Choose a reason for hiding this comment

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

should we make size a 16 bit quantity so it packs nicely into memory better? Oh I see it already does. Maybe add another uint16_t reserved so we can make use of those 16-bits and keep the padding alignment

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

uint16_t id; // Source ID
uint16_t type; // Data type
const char* name; // Source name
uint32_t flags; // Reserved (should be set to 0)
void* data; // User data
diag_source_cmd_callback callback; // Source callback
} diag_source;

typedef struct diag_source_get_cmd_data {
size_t size; // Size of this structure
void* data; // Data buffer
size_t data_size; // Buffer size
} diag_source_get_cmd_data;

int diag_register_source(const diag_source* src, void* reserved);
int diag_enum_sources(diag_enum_sources_callback callback, size_t* count, void* data, void* reserved);
int diag_get_source(uint16_t id, const diag_source** src, void* reserved);
int diag_service_cmd(int cmd, void* data, void* reserved);

#ifdef __cplusplus
} // extern "C"
#endif
6 changes: 5 additions & 1 deletion services/inc/services_dynalib.h
Expand Up @@ -67,7 +67,11 @@ DYNALIB_FN(33, services, led_set_update_enabled, void(int, void*))
DYNALIB_FN(34, services, led_update_enabled, int(void*))
DYNALIB_FN(35, services, led_update, void(system_tick_t, LEDStatusData*, void*))

DYNALIB_FN(36, services, diag_register_source, int(const diag_source*, void*))
DYNALIB_FN(37, services, diag_enum_sources, int(diag_enum_sources_callback, size_t*, void*, void*))
DYNALIB_FN(38, services, diag_get_source, int(uint16_t, const diag_source**, void*))
DYNALIB_FN(39, services, diag_service_cmd, int(int, void*, void*))

DYNALIB_END(services)

#endif /* SERVICES_DYNALIB_H */

131 changes: 131 additions & 0 deletions services/src/diagnostics.cpp
@@ -0,0 +1,131 @@
/*
* Copyright (c) 2017 Particle Industries, Inc. All rights reserved.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation, either
* version 3 of the License, or (at your option) any later version.
*
* This library is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this library; if not, see <http://www.gnu.org/licenses/>.
*/

#include "diagnostics.h"

#include "spark_wiring_vector.h"

#include "system_error.h"

#include <algorithm>

namespace {

using namespace spark;

class Diagnostics {
public:
Diagnostics() :
enabled_(0) { // The service is disabled initially
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the rationale for having enable/disable for the service? If it's to save expensive computation then perhaps we should move the enable flag to the diag_source?

Copy link
Member Author

Choose a reason for hiding this comment

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

The rationale is to avoid a race condition, which is possible when the system receives a request or a call querying diagnostic data but data sources are still being registered. The system explicitly enables the diagnostic service after invoking all global constructors in all system parts here

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that makes sense. The name enabled is slightly counter-intuitive and that you cannot add sources once it is enabled. On first reading, I was sure I understood it as the opposite - the error is raised when adding sources and the service is not enabled.

Perhaps we should call it running, or started? Alternatively, you could add a comment to explain, since normally things are not usable while disabled, while here it's the opposite.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

srcs_.reserve(32);
}

int registerSource(const diag_source* src) {
if (enabled_) {
return SYSTEM_ERROR_INVALID_STATE;
}
const int index = indexForId(src->id);
if (index < srcs_.size() && srcs_.at(index)->id == src->id) {
return SYSTEM_ERROR_ALREADY_EXISTS;
}
if (!srcs_.insert(index, src)) {
return SYSTEM_ERROR_NO_MEMORY;
}
return SYSTEM_ERROR_NONE;
}

int enumSources(diag_enum_sources_callback callback, size_t* count, void* data) {
if (!enabled_) {
return SYSTEM_ERROR_INVALID_STATE;
}
if (callback) {
for (const diag_source* src: srcs_) {
callback(src, data);
}
}
if (count) {
*count = srcs_.size();
}
return SYSTEM_ERROR_NONE;
}

int getSource(uint16_t id, const diag_source** src) {
if (!enabled_) {
return SYSTEM_ERROR_INVALID_STATE;
}
const int index = indexForId(id);
if (index >= srcs_.size() || srcs_.at(index)->id != id) {
return SYSTEM_ERROR_NOT_FOUND;
}
if (src) {
*src = srcs_.at(index);
}
return SYSTEM_ERROR_NONE;
}

int command(int cmd, void* data) {
switch (cmd) {
#if PLATFORM_ID == 3
// The RESET and DISABLE commands are used only for testing purposes
case DIAG_CMD_RESET:
srcs_.clear();
enabled_ = 0;
break;
case DIAG_CMD_DISABLE:
enabled_ = 0;
break;
#endif
case DIAG_CMD_ENABLE:
enabled_ = 1;
break;
default:
return SYSTEM_ERROR_NOT_SUPPORTED;
}
return SYSTEM_ERROR_NONE;
}

private:
Vector<const diag_source*> srcs_;
volatile uint8_t enabled_;

int indexForId(uint16_t id) const {
return std::distance(srcs_.begin(), std::lower_bound(srcs_.begin(), srcs_.end(), id,
[](const diag_source* src, uint16_t id) {
return (src->id < id);
}));
}
};

Diagnostics g_diag;

} // namespace

int diag_register_source(const diag_source* src, void* reserved) {
return g_diag.registerSource(src);
}

int diag_enum_sources(diag_enum_sources_callback callback, size_t* count, void* data, void* reserved) {
return g_diag.enumSources(callback, count, data);
}

int diag_get_source(uint16_t id, const diag_source** src, void* reserved) {
return g_diag.getSource(id, src);
}

int diag_service_cmd(int cmd, void* data, void* reserved) {
return g_diag.command(cmd, data);
}
3 changes: 2 additions & 1 deletion services/src/services_dynalib.c
Expand Up @@ -21,11 +21,12 @@
*/

#define DYNALIB_EXPORT

#include "rgbled.h"
#include "debug.h"
#include "jsmn.h"
#include "logging.h"
#include "system_error.h"
#include "led_service.h"
#include "diagnostics.h"
#include "services_dynalib.h"

4 changes: 4 additions & 0 deletions system/src/main.cpp
Expand Up @@ -46,6 +46,7 @@
#include "system_mode.h"
#include "rgbled.h"
#include "led_service.h"
#include "diagnostics.h"
#include "spark_wiring_power.h"
#include "spark_wiring_fuel.h"
#include "spark_wiring_interrupts.h"
Expand Down Expand Up @@ -652,6 +653,9 @@ void app_setup_and_loop(void)
system_power_management_init();
#endif

// Enable diagnostics
diag_service_cmd(DIAG_CMD_ENABLE, nullptr, nullptr);

DEBUG("Hello from Particle!");
String s = spark_deviceID();
INFO("Device %s started", s.c_str());
Expand Down
1 change: 1 addition & 0 deletions user/inc/application.h
Expand Up @@ -72,6 +72,7 @@
#include "spark_wiring_async.h"
#include "spark_wiring_error.h"
#include "spark_wiring_led.h"
#include "spark_wiring_diagnostics.h"
#include "fast_pin.h"
#include "string_convert.h"
#include "debug_output_handler.h"
Expand Down