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

Forwards logs to the server if server supports it #361

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/logging/LogQueue.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#include "LogQueue.h"

namespace SlimeVR::Logging
{

LogQueue LogQueue::s_Instance{};

} // namespace SlimeVR::Logging
111 changes: 111 additions & 0 deletions src/logging/LogQueue.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
#ifndef LOGGING_LOGQUEUE_H
#define LOGGING_LOGQUEUE_H

#include <array>
#include <cstdint>
#include <cstring>
#include <numeric>

namespace SlimeVR::Logging {

template <typename T, T Modulus>
class Modulo {
public:
Modulo(T value) : m_Value(value) {}

Modulo<T, Modulus>& operator++()
{
m_Value = (m_Value + 1) % Modulus;
return *this;
}

Modulo<T, Modulus> operator+(T other) const
{
// WARNING: Does not consider overflow or negative values
T newValue = (m_Value + other) % Modulus;
return Modulo<T, Modulus>{newValue};
}

T get() const
{
return m_Value;
}

private:
T m_Value;
};

class LogQueue {
public:
// Whether there are any messages in the queue.
bool empty() const
{
return m_Count == 0;
}

// First message in the queue.
const char* front() const
{
if (empty())
{
return nullptr;
}

return m_MessageQueue[m_StartIndex.get()].data();
}

// Adds a message to the end of the queue. Messages that are too long will be truncated.
void push(const char* message)
{
if (m_Count < m_MessageQueue.max_size())
{
setMessageAt(m_Count, message);
++m_Count;
}
else
{
// Overwrite the last message
setMessageAt(m_Count - 1, OverflowMessage);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would probably be a good idea to consider if overriding the first messages would make more sense in this context (tho honestly will never happen)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're probably right, it's better to keep the newest logs rather than the oldest logs.

Copy link
Contributor Author

@jabberrock jabberrock Nov 12, 2024

Choose a reason for hiding this comment

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

Do you think it's even necessary to send an [OVERFLOW] message? I'm worried once we get to overflow territory, every message we send to the server will just be [OVERFLOW].

Maybe if we can't keep up, we just silently drop (the oldest) messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed this by silently dropping earliest message.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think an overflow message would be particularly useful, and in theory it would override another message, would it not?

}
}

// Removes a message from the front of the queue.
void pop()
{
if (m_Count > 0)
{
++m_StartIndex;
--m_Count;
}
}

// Global instance of the log queue
static LogQueue& instance()
{
return s_Instance;
}

private:
static constexpr int MaxMessages = 100;
static constexpr int MaxMessageLength = std::numeric_limits<uint8_t>::max();
static constexpr char OverflowMessage[] = "[OVERFLOW]";

void setMessageAt(int offset, const char* message)
{
Modulo<size_t, MaxMessages> index = m_StartIndex + offset;
std::array<char, MaxMessageLength>& entry = m_MessageQueue[index.get()];

std::strncpy(entry.data(), message, entry.max_size());
entry[entry.max_size() - 1] = '\0'; // NULL terminate string in case message overflows because strncpy does not do that
}

Modulo<size_t, MaxMessages> m_StartIndex{0};
size_t m_Count = 0;
std::array<std::array<char, MaxMessageLength>, MaxMessages> m_MessageQueue;

static LogQueue s_Instance;
};

} // namespace SlimeVR::Logging

#endif /* LOGGING_LOGQUEUE_H */
4 changes: 4 additions & 0 deletions src/logging/Logger.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "Logger.h"
#include "LogQueue.h"

namespace SlimeVR {
namespace Logging {
Expand Down Expand Up @@ -65,6 +66,9 @@ void Logger::log(Level level, const char* format, va_list args) {
}

Serial.printf("[%-5s] [%s] %s\n", levelToString(level), buf, buffer);

// REVIEW: Do we want to print the log level and tags too?
LogQueue::instance().push(buffer);
Comment on lines +70 to +71
Copy link
Member

Choose a reason for hiding this comment

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

REVIEW: Do we want to print the log level and tags too?

We probably want identical output to serial here.

}
} // namespace Logging
} // namespace SlimeVR
37 changes: 36 additions & 1 deletion src/network/connection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

#include "GlobalVars.h"
#include "logging/Logger.h"
#include "logging/LogQueue.h"
#include "packets.h"

#define TIMEOUT 3000UL
Expand Down Expand Up @@ -189,7 +190,12 @@ bool Connection::sendPacketNumber() {
}

bool Connection::sendShortString(const char* str) {
uint8_t size = strlen(str);
size_t size = strlen(str);

constexpr size_t maxLength = std::numeric_limits<uint8_t>::max();
if (size > maxLength) {
size = maxLength;
}

MUST_TRANSFER_BOOL(sendByte(size));
MUST_TRANSFER_BOOL(sendBytes((const uint8_t*)str, size));
Expand Down Expand Up @@ -395,6 +401,31 @@ void Connection::sendAcknowledgeConfigChange(uint8_t sensorId, uint16_t configTy
MUST(endPacket());
}

// PACKET_LOG 26
void Connection::sendLogMessage(const char* message) {
MUST(m_Connected);

MUST(beginPacket());

MUST(sendPacketType(PACKET_LOG));
MUST(sendPacketNumber());
MUST(sendShortString(message));

MUST(endPacket());
}

void Connection::sendPendingLogMessage() {
MUST(m_Connected);
MUST(m_ServerFeatures.has(ServerFeatures::PROTOCOL_LOG_SUPPORT));

Logging::LogQueue& logQueue = Logging::LogQueue::instance();
if (!logQueue.empty()) {
const char* message = logQueue.front();
sendLogMessage(message);
logQueue.pop();
}
}

void Connection::sendTrackerDiscovery() {
MUST(!m_Connected);

Expand Down Expand Up @@ -634,6 +665,7 @@ void Connection::update() {
auto& sensors = sensorManager.getSensors();

updateSensorState(sensors);
sendPendingLogMessage();
maybeRequestFeatureFlags();

if (!m_Connected) {
Expand Down Expand Up @@ -731,6 +763,9 @@ void Connection::update() {
m_Logger.debug("Server supports packet bundling");
}
#endif
if (m_ServerFeatures.has(ServerFeatures::PROTOCOL_LOG_SUPPORT)) {
m_Logger.debug("Server supports network logging");
}
}

break;
Expand Down
4 changes: 4 additions & 0 deletions src/network/connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,10 @@ class Connection {

void sendAcknowledgeConfigChange(uint8_t sensorId, uint16_t configType);

// PACKET_LOG 26
void sendLogMessage(const char* message);
void sendPendingLogMessage();

bool m_Connected = false;
SlimeVR::Logging::Logger m_Logger = SlimeVR::Logging::Logger("UDPConnection");

Expand Down
8 changes: 8 additions & 0 deletions src/network/featureflags.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ struct ServerFeatures {
// Server can parse bundle packets: `PACKET_BUNDLE` = 100 (0x64).
PROTOCOL_BUNDLE_SUPPORT,

// Server can parse bundle packets with compact headers and packed IMU rotation/acceleration frames:
// - `PACKET_BUNDLE_COMPACT` = 101 (0x65),
// - `PACKET_ROTATION_AND_ACCELERATION` = 23 (0x17).
PROTOCOL_BUNDLE_COMPACT_SUPPORT,
Copy link
Contributor Author

Choose a reason for hiding this comment

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


// Server can receive log messages: `PACKET_LOG` = 102 (0x66).
PROTOCOL_LOG_SUPPORT,

// Add new flags here

BITS_TOTAL,
Expand Down
1 change: 1 addition & 0 deletions src/network/packets.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
// packet
#define PACKET_ACKNOWLEDGE_CONFIG_CHANGE 24
#define PACKET_SET_CONFIG_FLAG 25
#define PACKET_LOG 26

#define PACKET_BUNDLE 100

Expand Down