-
-
Notifications
You must be signed in to change notification settings - Fork 308
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
jabberrock
wants to merge
4
commits into
SlimeVR:main
Choose a base branch
from
jabberrock:jabber-forward-logs
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
7bf1a50
Forwards logs to the server if server supports it
jabberrock 71fa933
Move LogQueue implementation into .cpp and add copyright preamble
jabberrock 804c151
Silently drop earliest message if LogQueue is full, remove Module hel…
jabberrock 6061ba3
Reduce number of messages in LogQueue to 30 to conserve RAM
jabberrock File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
/* | ||
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].data(); | ||
} | ||
|
||
void LogQueue::push(const char* message) | ||
{ | ||
if (m_Count >= m_MessageQueue.max_size()) | ||
{ | ||
// Drop the earliest message to make space | ||
pop(); | ||
} | ||
|
||
setMessageAt(m_Count, message); | ||
++m_Count; | ||
} | ||
|
||
void LogQueue::pop() | ||
{ | ||
if (m_Count > 0) | ||
{ | ||
m_StartIndex = (m_StartIndex + 1) % m_MessageQueue.max_size(); | ||
--m_Count; | ||
} | ||
} | ||
|
||
void LogQueue::setMessageAt(int offset, const char* message) | ||
{ | ||
size_t index = (m_StartIndex + offset) % m_MessageQueue.max_size(); | ||
std::array<char, MaxMessageLength>& entry = m_MessageQueue[index]; | ||
|
||
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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
/* | ||
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. | ||
// - If the queue is full, the earliest message will be silently dropped | ||
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 = 30; | ||
static constexpr size_t MaxMessageLength = std::numeric_limits<uint8_t>::max(); | ||
|
||
size_t 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 */ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
#include "Logger.h" | ||
#include "LogQueue.h" | ||
|
||
namespace SlimeVR { | ||
namespace Logging { | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We probably want identical output to serial here. |
||
} | ||
} // namespace Logging | ||
} // namespace SlimeVR |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 inConnections::sendShortString
limits the string that can be sent on that wire format.It feels weird for
Connections
to reference logging size, or forLogQueue
to reference wire format size.There was a problem hiding this comment.
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 usestd::numeric_limits<ulong_t>::max()
for ourMaxMessageLength
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this the way it is, as this may control the memory usage and is a decided restriction, whereas the one in
Connection::sendShortString()
is a restriction enforced by the protocol.