Rev 1 #7

OcelotEmpire · 2024-01-20T21:08:04Z

No description provided.

Changes ported from High-Level-RoveComm-for-C++-Development branch

Moved functions into proper header files and reformatted code into correct documentation

Added semicolon to end of class to resolve issue

GetManifest is no longer a required function

…RDT/RoveComm_CPP into topic/rovecomm-cpp-dev

…I can implement singlethreading first.

…fer.

…veComm_CPP into topic/adam-edits

ClayJay3

Nice job on being the first one to have a PR! A majority of the code needs comments, you should be able to read through your comments in English and understand what the code does. Who knows, you might catch a few bugs just going back through and commenting everything! Testing to make sure everything works is needed.

It also appears that the check are failing now. It seems there's a multiple definition of a lock_guard somewhere.

src/RoveComm/Consts.h

src/RoveComm/RoveCommConstants.h

ClayJay3 · 2024-01-20T23:15:10Z

src/RoveComm/RoveCommConstants.h

+
+namespace rovecomm
+{
+    const int ROVECOMM_ETHERNET_UDP_MAX_SUBSCRIBERS = 10;


Add a comment to the end of each line explaining what the constants does. Similar to how AutonomyConstants.h is written.

ClayJay3 · 2024-01-20T23:15:59Z

src/RoveComm/RoveCommConstants.h

+    namespace System
+    {
+        // pass to RoveCommServer::Fetch() signifying any data id. This is not a valid data id.
+        const int ANY = 0;


This constant name is not very descriptive.

ClayJay3 · 2024-01-20T23:16:31Z

src/RoveComm/RoveCommConstants.h

+    const int ROVECOMM_VERSION                      = 3;
+    const int ROVECOMM_ETHERNET_TCP_MAX_CONNECTIONS = 5;
+
+    namespace System


Why is this namespace here? Needs docs to explain what it is for.

If you are going to keep this namespace make sure it is written in lowercase.

src/RoveComm/RoveCommServer.h

src/RoveCommGlobals.cpp

src/RoveCommGlobals.h

src/RoveCommLogging.cpp

src/RoveCommLogging.h

Byrdman32

When writing log statements, think about what you would want to see printed to the console when running an application that includes RoveComm. At the current moment, I am thinking that there is going to be a lot of excess data in the console so I would advise making some of the logs that are currently, LOG_INFO be LOG_DEBUG so that when everything is working well we don't have an overwhelmed console.

Overall, just make sure to review the API Docs, https://missourimrdt.github.io/Autonomy_Software/md_CONTRIBUTING.html, for Autonomy to make sure that your naming conventions are correct.

src/RoveComm/Consts.h

src/RoveComm/LogHack.h

Byrdman32 · 2024-01-24T23:06:42Z

src/RoveComm/RoveComm.h

+#include "RoveCommPacket.h"
+#include "RoveCommServer.h"
+
+extern RoveCommServerManager RoveComm;


The purpose of this file is you include "RoveComm.h" in your external project however, the RoveComm Object shouldn't be created here.

Byrdman32 · 2024-01-24T23:53:07Z

src/RoveComm/RoveCommServer.h

+ * @brief Base class for network thread management.
+ *
+ * @file RoveCommPacket.h
+ * @author Eli Byrd ([email protected])


Add yourself as an author to this header.

Byrdman32 · 2024-01-24T23:53:51Z

src/RoveComm/RoveCommServer.h

+
+// TODO: make this a real singleton ^^^
+
+#endif


Add guard lock comment

Byrdman32 · 2024-01-24T23:54:24Z

src/RoveCommGlobals.cpp

@@ -1,89 +1,10 @@
-/******************************************************************************


This header should be added back.

Byrdman32 · 2024-01-24T23:54:38Z

src/RoveCommGlobals.cpp

- *
- *
- * @file RoveCommGlobals.cpp
- * @author Eli Byrd ([email protected])


But also add your name as an author

Byrdman32 · 2024-01-24T23:55:21Z

src/RoveCommLogging.cpp

+ *              distributed into other projects.
+ *
+ * @file AutonomyLogging.cpp
+ * @author Eli Byrd ([email protected])


Add yourself as an author to this header.

ThisIsBrady · 2024-01-31T00:33:39Z

src/RoveComm/RoveCommEthernetTcp.cpp

+    // result will actually be a linked list but for now we just get the first entry
+    if (int status = getaddrinfo(NULL, std::to_string(m_unPort).c_str(), &hints, &result) != 0)
+    {
+        LOG_ERROR(logging::g_qSharedLogger, "Failed to find IP! Error: {}", gai_strerror(status));


This function returns a human-readable description of the error that corresponds to the status. I would recommend keeping it and potentially also printing the integer status.

src/RoveComm/RoveCommEthernetTcp.cpp

ThisIsBrady · 2024-01-31T01:08:19Z

src/RoveComm/RoveCommEthernetTcp.cpp

+            close(m_nListeningSocket);
+            continue;
+        }
+        break;


This break means that the for loop will only execute a single iteration

src/RoveComm/RoveCommEthernetTcp.cpp

ThisIsBrady · 2024-01-31T02:36:44Z

src/RoveComm/RoveCommEthernetUdp.cpp

+    // result will actually be a linked list but for now we just get the first entry
+    if (int status = getaddrinfo(NULL, std::to_string(m_unPort).c_str(), &hints, &result) != 0)
+    {
+        LOG_ERROR(logging::g_qSharedLogger, "Failed to find IP! Error: {}", gai_strerror(status));


function returns human readable description of error instead of just the integer status. I'd recommend keeping it.

ThisIsBrady · 2024-01-31T02:44:31Z

src/RoveComm/RoveCommEthernetUdp.cpp

+
+    if (int status = getaddrinfo(address.GetIp().ToString().c_str(), std::to_string(m_unPort).c_str(), &hints, &result) != 0)
+    {
+        LOG_ERROR(logging::g_qSharedLogger, "Failed to find IP! Error: {}", gai_strerror(status));


Produces human-readable error message instead of integer status code.

src/RoveComm/RoveCommEthernetUdp.cpp

src/RoveComm/RoveCommServer.cpp

ClayJay3 · 2024-03-21T15:21:17Z

Closing the PR for now, comments are still available. Reopen or recreate if you want to finish merging.

Viktor-Butkovich and others added 24 commits October 8, 2023 19:20

Initial commit

ae28aaa

Changes ported from High-Level-RoveComm-for-C++-Development branch

Moved class headers

bb4a456

Updated formatting

b564a02

Reformatting

5c25b82

Moved functions into proper header files and reformatted code into correct documentation

Worked on struct

2cee04a

Created ethernet member variables

bb13c68

Fixed error in RoveComm.h

552301b

Added semicolon to end of class to resolve issue

Started RoveComm, TCP function definitions

22107eb

Started remaining function definitions

fa63512

Started RoveComm constructor

7643fed

Continued RoveComm function implementation

f2e7f2b

Got rid of GetManifest

4c8b220

GetManifest is no longer a required function

Partially implemented callback functions

52cb88c

Merge branch 'topic/rovecomm-cpp-dev' of https://github.com/MissouriM…

36e6e51

…RDT/RoveComm_CPP into topic/rovecomm-cpp-dev

Fixing a lot of errors

0df2046

Fixed function defaults on RoveCommPacket

85ffd5c

Updated RovecommPacket to get data array

4ce734d

Added software lead edits

e5b265d

Merge branch 'topic/software-lead-edits' into topic/rovecomm-cpp-dev

cbfa8a2

Partial TCP socket implementation

74d40e1

Implemented Write function

b65a892

Implemented Read and Handle functions

ade7a61

Started UDP and added functions to RoveCommPacket

43d6220

Merge branch 'topic/rovecomm-cpp-dev' into topic/adam-edits

f9dbd67

OcelotEmpire requested review from ClayJay3 and Byrdman32 January 20, 2024 21:08

OcelotEmpire self-assigned this Jan 20, 2024

OcelotEmpire added 3 commits January 20, 2024 17:46

created a server abstraction to allow additions of more protocols

c6ddbcb

added a type to make packet reading more convenient

243b793

added AutonomyThread.hpp header

83de610

OcelotEmpire and others added 15 commits January 20, 2024 19:18

created a server abstraction to allow additions of more protocols

f4d88b1

added a type to make packet reading more convenient

f06d5de

added AutonomyThread.hpp header

eaa7b76

moved boilerplate to RoveCommHelpers.h and removed multithreading so …

497710d

…I can implement singlethreading first.

implemented TCP initialization

1677888

got select() working hopefully. just need to implement a circular buf…

59590ef

…fer.

deleted RoveCommByteBuffer

a26aab0

finished TCP and most of UDP

16f0026

finally done! Fetch() has not yet been implemented though.

6d21c4f

added stuff to main.cpp

6edff90

fixed logging

89cbc05

added loghack.h

b8c46cf

Update Dev Container

2ded9d9

Merge branch 'topic/adam-edits' of https://github.com/MissouriMRDT/Ro…

0db8c0c

…veComm_CPP into topic/adam-edits

minor bug fixes

b6c1e1f

Byrdman32 force-pushed the topic/adam-edits branch from 572ff6d to b6c1e1f Compare January 21, 2024 01:23

ClayJay3 requested changes Jan 21, 2024

View reviewed changes

ClayJay3 requested a review from ThisIsBrady January 24, 2024 00:14

Byrdman32 requested changes Jan 25, 2024

View reviewed changes

Viktor-Butkovich added 2 commits January 31, 2024 00:15

Rearranged status conditionals

5ab4de7

Removed assignment operators from conditionals

a330c8c

ThisIsBrady suggested changes Jan 31, 2024

View reviewed changes

OcelotEmpire added 6 commits February 3, 2024 22:37

removed annoying type aliases and removed Fetch() function

61fba82

added functional programming to make things function better

7300bcb

removed ipv6 support to make the code more readable

f5ccd10

added code to actually call requests in the command queue

88ddb61

moved ServerManager to its own file and removed the packet queue

5740fbf

moved LogHack.h and ThreadHack.h into LibraryImports.h

b0fd93e

ClayJay3 closed this Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rev 1 #7

Rev 1 #7

OcelotEmpire commented Jan 20, 2024

ClayJay3 left a comment •

edited

Loading

ClayJay3 Jan 20, 2024

ClayJay3 Jan 20, 2024

ClayJay3 Jan 20, 2024

Byrdman32 Jan 24, 2024

Byrdman32 left a comment

Byrdman32 Jan 24, 2024

Byrdman32 Jan 24, 2024

Byrdman32 Jan 24, 2024

Byrdman32 Jan 24, 2024

Byrdman32 Jan 24, 2024

Byrdman32 Jan 24, 2024

ThisIsBrady Jan 31, 2024

ThisIsBrady Jan 31, 2024

ThisIsBrady Jan 31, 2024

ThisIsBrady Jan 31, 2024

ClayJay3 commented Mar 21, 2024

		@@ -1,89 +1,10 @@
		/******************************************************************************

Rev 1 #7

Rev 1 #7

Conversation

OcelotEmpire commented Jan 20, 2024

ClayJay3 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Byrdman32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ClayJay3 commented Mar 21, 2024

ClayJay3 left a comment •

edited

Loading