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 2 commits
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
84 changes: 84 additions & 0 deletions src/logging/LogQueue.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
SlimeVR Code is placed under the MIT license
Copyright (c) 2024 Jabberrock & SlimeVR contributors

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in
all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.
*/

#include "LogQueue.h"

namespace SlimeVR::Logging
{

bool LogQueue::empty() const
{
return m_Count == 0;
}

const char* LogQueue::front() const
{
if (empty())
{
return nullptr;
}

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

void LogQueue::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.

It's probably worth considering, if maybe overriding the oldest message would make more sense in this context (though it will never happen hopefully).

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.

}
}

void LogQueue::pop()
{
if (m_Count > 0)
{
++m_StartIndex;
--m_Count;
}
}

void LogQueue::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
}

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

LogQueue LogQueue::s_Instance{};

} // namespace SlimeVR::Logging
95 changes: 95 additions & 0 deletions src/logging/LogQueue.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
/*
SlimeVR Code is placed under the MIT license
Copyright (c) 2024 Jabberrock & SlimeVR contributors

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in
all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.
*/

#ifndef LOGGING_LOGQUEUE_H
#define LOGGING_LOGQUEUE_H

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

namespace SlimeVR::Logging {

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

// First message in the queue.
const char* front() const;

// Adds a message to the end of the queue. Messages that are too long will be truncated.
void push(const char* message);

// Removes a message from the front of the queue.
void pop();

// Global instance of the log queue
static LogQueue& instance();

private:
// Sets the message at a particular offset
void setMessageAt(int offset, const char* message);

static constexpr size_t MaxMessages = 100;
static constexpr size_t MaxMessageLength = std::numeric_limits<uint8_t>::max();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have this defined as a publicly available constexpr variable inside logger or something, that way it isn't duplicated in multiple cases of the codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't really feel duplicated. This constant here limits the size of the LogQueue. The check I added in Connections::sendShortString limits the string that can be sent on that wire format.

It feels weird for Connections to reference logging size, or for LogQueue to reference wire format size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

E.g. If we wanted to send longer messages, then we'd have to switch to Connections::sendLongString, which has a very different limit. We wouldn't use std::numeric_limits<ulong_t>::max() for our MaxMessageLength.

static constexpr char OverflowMessage[] = "[OVERFLOW]";

template <typename T, T Modulus>
class Modulo {
unlogisch04 marked this conversation as resolved.
Show resolved Hide resolved
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;
};

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);
}
} // 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