Added a name only constructor to ComLogger #1983

kubiak-jpl · 2023-04-24T19:53:15Z


*Originating Project/Creator*	@kubiak-jpl
*Affected Component*	ComLogger
*Affected Architectures(s)*	N/A
*Related Issue(s)*	N/A
*Has Unit Tests (y/n)*	y
*Builds Without Errors (y/n)*	y
*Unit Tests Pass (y/n)*	y
*Documentation Included (y/n)*	n

Change Description

Updated ComLogger to include a name-only constructor and an additional initialization method

ComLogger(const char* compName);

      // filePrefix: string to prepend the file name with, ie. "thermal_telemetry"
      // maxFileSize: the maximum size a file should reach before being closed and a new one opened
      // storeBufferLength: if true, store the length of each com buffer before storing the buffer itself,
      //                    otherwise just store the com buffer. false might be advantageous in a system
      //                    where you can ensure that all buffers given to the ComLogger are the same size
      //                    in which case you do not need the overhead. Or you store an id which you can
      //                    match to an expected size on the ground during post processing.
      void init_log_file(const char* filePrefix, U32 maxFileSize, bool storeBufferLength=true);

Added a new internal state to keep track of initialization. The ComLogger file will not open if init_log_file is not called and a WARNING_LO event will be created. Using the old constructor sets this state to initialized by default

Added unit tests to cover the new constructor.

Rationale

The name-only constructor is necessary to use ComLogger without the need for phases in the topology fpp.

Testing/Review Recommendations

Updated unit tests

Future Work

None

Svc/ComLogger/ComLogger.hpp

@@ -52,11 +52,23 @@
      //                    match to an expected size on the ground during post processing.
      ComLogger(const char* compName, const char* filePrefix, U32 maxFileSize, bool storeBufferLength=true);

+      // CONSTRUCTOR:
+      ComLogger(const char* compName);


Svc/ComLogger/ComLogger.cpp

Svc/ComLogger/ComLogger.hpp

@@ -112,6 +124,7 @@
      bool writeErrorOccurred;
      bool openErrorOccurred;
      bool storeBufferLength;
+      bool initialized;


Svc/ComLogger/ComLogger.cpp

+  void ComLogger ::
+    init_log_file(const char* incomingFilePrefix, U32 maxFileSize, bool storeBufferLength)
+  {
+    this->maxFileSize = maxFileSize;


Svc/ComLogger/ComLogger.cpp

+    init_log_file(const char* incomingFilePrefix, U32 maxFileSize, bool storeBufferLength)
+  {
+    this->maxFileSize = maxFileSize;
+    this->storeBufferLength = storeBufferLength;


Svc/ComLogger/ComLogger.cpp

LeStarch

Minor comments, but looks correct!

Svc/ComLogger/ComLogger.cpp

+  ComLogger ::
+    ComLogger(const char* compName) :


Svc/ComLogger/ComLogger.cpp

  }

+  ComLogger ::
+    ComLogger(const char* compName) :


Svc/ComLogger/ComLogger.cpp

@@ -49,6 +67,25 @@
    ComLoggerComponentBase::init(queueDepth, instance);
  }

+  void ComLogger ::
+    init_log_file(const char* incomingFilePrefix, U32 maxFileSize, bool storeBufferLength)


Svc/ComLogger/ComLogger.cpp

@@ -49,6 +67,25 @@
    ComLoggerComponentBase::init(queueDepth, instance);
  }

+  void ComLogger ::
+    init_log_file(const char* incomingFilePrefix, U32 maxFileSize, bool storeBufferLength)


Svc/ComLogger/ComLogger.cpp

+  void ComLogger ::
+    init_log_file(const char* incomingFilePrefix, U32 maxFileSize, bool storeBufferLength)


LeStarch · 2023-05-11T00:02:51Z

@kubiak-jpl this looks good. I did three things:

Initialized the other missing variables
added a not-null assert
From the with-arguments constructor, I delegated to your new init function to prevent code duplication.

If you are ok with this, we can merge!

Svc/ComLogger/ComLogger.cpp

-    }
-    FW_ASSERT(Fw::StringUtils::string_length(incomingFilePrefix, sizeof(this->filePrefix)) < sizeof(this->filePrefix),
-      Fw::StringUtils::string_length(incomingFilePrefix, sizeof(this->filePrefix)), sizeof(this->filePrefix)); // ensure that file prefix is not too big
+    this->init_log_file(incomingFilePrefix, maxFileSize, storeBufferLength);


Svc/ComLogger/ComLogger.cpp

-    }
-    FW_ASSERT(Fw::StringUtils::string_length(incomingFilePrefix, sizeof(this->filePrefix)) < sizeof(this->filePrefix),
-      Fw::StringUtils::string_length(incomingFilePrefix, sizeof(this->filePrefix)), sizeof(this->filePrefix)); // ensure that file prefix is not too big
+    this->init_log_file(incomingFilePrefix, maxFileSize, storeBufferLength);


Svc/ComLogger/ComLogger.cpp

-    }
-    FW_ASSERT(Fw::StringUtils::string_length(incomingFilePrefix, sizeof(this->filePrefix)) < sizeof(this->filePrefix),
-      Fw::StringUtils::string_length(incomingFilePrefix, sizeof(this->filePrefix)), sizeof(this->filePrefix)); // ensure that file prefix is not too big
+    this->init_log_file(incomingFilePrefix, maxFileSize, storeBufferLength);


kubiak-jpl · 2023-05-11T15:22:49Z

@kubiak-jpl this looks good. I did three things:

Initialized the other missing variables

added a not-null assert

From the with-arguments constructor, I delegated to your new init function to prevent code duplication.

If you are ok with this, we can merge!

Looks good to me

LeStarch · 2023-05-16T16:55:16Z

@kubiak-jpl merged!

* Added a name only constructor to ComLogger * Minor fixup of CodeQL warnings * Fixing ctor warnings, checking arg * Reusing init_log_file in constructor * sp * NULL -> nullptr * Adding throttle --------- Co-authored-by: M Starch <LeStarch@googlemail.com>

This reverts commit d075381.

Added a name only constructor to ComLogger

b45293b

github-advanced-security bot found potential problems Apr 26, 2023

View reviewed changes

Svc/ComLogger/ComLogger.cpp Fixed Show fixed Hide fixed

Svc/ComLogger/ComLogger.cpp Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Apr 26, 2023

View reviewed changes

Svc/ComLogger/ComLogger.cpp Fixed Show resolved Hide resolved

Svc/ComLogger/ComLogger.cpp Fixed Show fixed Hide fixed

Svc/ComLogger/ComLogger.cpp Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Apr 26, 2023

View reviewed changes

LeStarch reviewed Apr 27, 2023

View reviewed changes

Minor fixup of CodeQL warnings

528a78b

github-advanced-security bot found potential problems Apr 27, 2023

View reviewed changes

Svc/ComLogger/ComLogger.cpp

Comment on lines +44 to +45

ComLogger ::

ComLogger(const char* compName) :

Check notice

Code scanning / CodeQL

Long function without assertion

All functions of more than 10 lines should have at least one assertion.

github-advanced-security bot found potential problems Apr 27, 2023

View reviewed changes

Svc/ComLogger/ComLogger.cpp

}

ComLogger ::

ComLogger(const char* compName) :

Check notice

Code scanning / CodeQL

Use of basic integral type

compName uses the basic integral type char rather than a typedef with size and signedness.

github-advanced-security bot found potential problems Apr 27, 2023

View reviewed changes

Svc/ComLogger/ComLogger.cpp Fixed Show fixed Hide fixed

Svc/ComLogger/ComLogger.cpp Fixed Show fixed Hide fixed

Fixing ctor warnings, checking arg

54a5cf4

github-advanced-security bot found potential problems May 10, 2023

View reviewed changes

Reusing init_log_file in constructor

5bd2ac3

LeStarch approved these changes May 11, 2023

View reviewed changes

sp

8fbc604

github-advanced-security bot found potential problems May 11, 2023

View reviewed changes

NULL -> nullptr

bf7bbfc

Adding throttle

45be63c

LeStarch merged commit 08a2c68 into nasa:devel May 16, 2023

thomas-bc added a commit that referenced this pull request Aug 4, 2023

Revert "Added a name only constructor to ComLogger (#1983)"

5905e9a

This reverts commit d075381.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a name only constructor to ComLogger #1983

Added a name only constructor to ComLogger #1983

kubiak-jpl commented Apr 24, 2023

LeStarch left a comment

LeStarch commented May 11, 2023

kubiak-jpl commented May 11, 2023

LeStarch commented May 16, 2023

		void ComLogger ::
		init_log_file(const char* incomingFilePrefix, U32 maxFileSize, bool storeBufferLength)

Added a name only constructor to ComLogger #1983

Added a name only constructor to ComLogger #1983

Conversation

kubiak-jpl commented Apr 24, 2023

Change Description

Rationale

Testing/Review Recommendations

Future Work

LeStarch left a comment

Choose a reason for hiding this comment

LeStarch commented May 11, 2023

kubiak-jpl commented May 11, 2023

LeStarch commented May 16, 2023