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

[datalog] Move DataLogReaderThread from glass to new libdatalog #7641

Open
wants to merge 59 commits into
base: 2027
Choose a base branch
from

Conversation

DeltaDizzy
Copy link
Contributor

@DeltaDizzy DeltaDizzy commented Jan 5, 2025

Currently the major DataLog backend API (reading and writing) is split between wpiutil and glass. In the interest of allowing code that wants to use these APIs to not need to link to glass and declutter wpiutil, all of those APIs are moved to a new library named "libdatalog".

@DeltaDizzy DeltaDizzy requested review from PeterJohnson and a team as code owners January 5, 2025 18:08
@github-actions github-actions bot added component: wpiutil WPI utility library component: sysid SysId app component: datalogtool DataLog Tool labels Jan 5, 2025
@DeltaDizzy DeltaDizzy force-pushed the datalogreader-to-wpiutil branch 2 times, most recently from 92897bb to d429635 Compare January 8, 2025 20:54
@DeltaDizzy DeltaDizzy changed the base branch from main to 2027 January 8, 2025 20:59
@DeltaDizzy DeltaDizzy requested review from a team as code owners January 8, 2025 20:59
@DeltaDizzy DeltaDizzy changed the title [wpiutil] Move DataLogReaderThread from glass to wpiutil [datalog] Move DataLogReaderThread from glass to new libdatalog Jan 8, 2025
@github-actions github-actions bot added component: cscore CameraServer library component: wpilibc WPILib C++ component: hal Hardware Abstraction Layer component: command-based WPILib Command Based Library component: wpimath Math library component: glass Glass app and backend component: apriltag AprilTag library component: epilogue Annotation-based logging library 2027 2027 target labels Jan 8, 2025
@KangarooKoala
Copy link
Contributor

KangarooKoala commented Jan 9, 2025

Try doing git rebase -i 2027 and replacing pick with drop for the commits not in this PR.

If that doesn't work, try git reset --hard 92897bbc7156ece6d2433c2ecec6d3bc66bd6a9d (to reset to before the rebase) and then git rebase --onto 2027 HEAD main (to rebase while excluding the commits to main that aren't in 2027).

@DeltaDizzy DeltaDizzy force-pushed the datalogreader-to-wpiutil branch from f2ab2ec to d45aec8 Compare January 9, 2025 00:53
@DeltaDizzy DeltaDizzy requested a review from a team as a code owner January 9, 2025 17:31
@github-actions github-actions bot added component: ntcore NetworkTables library component: wpilibj WPILib Java labels Jan 9, 2025
ntcore/build.gradle Outdated Show resolved Hide resolved
ntcore/build.gradle Outdated Show resolved Hide resolved
@DeltaDizzy DeltaDizzy force-pushed the datalogreader-to-wpiutil branch from 894c428 to 9f02966 Compare January 28, 2025 02:52
@@ -21,6 +21,7 @@
#define _WIN32_WINNT 0x0601
#define _WIN32_IE 0x0800 // MinGW at it again. FIXME: verify if still needed.
#define WIN32_LEAN_AND_MEAN
#define kInvalidFile reinterpret_cast<void*>(-1)
Copy link
Member

Choose a reason for hiding this comment

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

Let's make a different PR for the fs.h/cpp change. Also, this file should be updated to use the new defined values in fs.h instead of replicating those changes here.

@@ -19,15 +19,15 @@
#include <vector>
Copy link
Member

@PeterJohnson PeterJohnson Jan 28, 2025

Choose a reason for hiding this comment

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

I think as we're moving these headers, we should put them in either wpi/log or wpi/datalog instead of the top level wpi. wpi/log would match the namespace, but datalog would be more consistent with struct and protobuf and the library name, etc. This would avoid the risk of conflicts with wpiutil since they're in two separate source directories?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the namespace be changed to match as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2027 2027 target component: apriltag AprilTag library component: command-based WPILib Command Based Library component: cscore CameraServer library component: datalogtool DataLog Tool component: epilogue Annotation-based logging library component: glass Glass app and backend component: hal Hardware Abstraction Layer component: ntcore NetworkTables library component: sysid SysId app component: wpilibc WPILib C++ component: wpilibj WPILib Java component: wpimath Math library component: wpiutil WPI utility library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants