Trace Logger Feature Review and Merge #2953

Shivaly-Reddy · 2024-10-15T19:10:33Z


*Related Issue(s)*	FP-2784
*Has Unit Tests (y/n)*	Y
*Documentation Included (y/n)*	Y

Change Description

This PR is to merge the initial Trace logger into F-Prime.

Rationale

To add trace logging capabilities to capture a snapshot in time that will help with debugging software execution

Testing/Review Recommendations

Added UTs to test trace logging and filtering features along with commands.

Future Work

This is just a beginning of implementation of trace feature, the future work would include logging in memory (instead of a file), implementing platform specific logging feature (vxWorks for example) and being able to dump data products.

This reverts commit 9b1cd59.

…nd Michael" This reverts commit 5d50140.

…hpp" This reverts commit d1c48ca.

This reverts commit 8d30c55.

This reverts commit 25ede54.

This reverts commit 2e67154.

This reverts commit 2a5dba3.

…devel

Fw/Trace/TraceBuffer.cpp

+    TraceBuffer::~TraceBuffer() {
+    }
+
+    TraceBuffer::TraceBuffer(const TraceBuffer& other) : Fw::SerializeBufferBase() {


Fw/Trace/TraceBuffer.hpp

github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Fw/Trace/TraceEntity.cpp

Svc/TraceFileLogger/ArrayProc.cpp

+        return false;
+    }
+
+    bool ArrayProc::delete_element(U32 element) {


Svc/TraceFileLogger/ArrayProc.cpp

Svc/TraceFileLogger/TraceFileLogger.cpp

+    // Component construction and destruction
+    // ----------------------------------------------------------------------
+
+    TraceFileLogger::TraceFileLogger(const char* const compName) : 


Svc/TraceFileLogger/TraceFileLogger.cpp

+            TraceFileLoggerComponentBase::init(queueDepth,instance);
+    }
+
+    void TraceFileLogger::set_log_file(const char* fileName, const U32 maxSize) {


Svc/TraceFileLogger/TraceFileLogger.cpp

+
+    // The filter method selects which trace types to be logged and which to ignore.
+    // The bit mask is based on the enum list for trace type in TraceCfg.fpp
+     Fw::CmdResponse TraceFileLogger::filterTraceType(U16 traceType_bitmask, bool enable){


Svc/TraceFileLogger/TraceFileLogger.cpp

+
+    }
+
+    void TraceFileLogger::write_log_file(U8* data, U32 size) {


Svc/TraceFileLogger/TraceFileLogger.cpp

+        }
+    }
+
+    Fw::CmdResponse TraceFileLogger::process_traceId_storage(U32 traceId,bool enable) {


Svc/TraceFileLogger/TraceFileLogger.cpp

+    // Handler implementations for user-defined typed input ports
+    // ----------------------------------------------------------------------
+
+    void TraceFileLogger::TraceBufferLogger_handler(FwIndexType portNum,


timcanham

Some changes...

timcanham · 2024-10-17T04:06:48Z

Fw/Com/ComPacket.hpp

@@ -26,6 +26,7 @@ namespace Fw {
                FW_PACKET_PACKETIZED_TLM, // !< Packetized telemetry packet type
                FW_PACKET_DP, //!< Data product packet
                FW_PACKET_IDLE, // !< Idle packet
+                FW_PACKET_TRACE, // !< Trace packet


I don't think we need a new packet type. Where is it used? The traces would come down as a data product.

It was part of initial implementation . Removed it.

Fw/Trace/TraceBuffer.cpp

timcanham · 2024-10-17T04:12:52Z

Fw/Trace/TraceBuffer.cpp

+        //FW_ASSERT(text != nullptr);
+        FW_ASSERT(this->getBuffLength() < FW_TRACE_BUFFER_MAX_SIZE);
+
+        std::string str_format(reinterpret_cast<const char*>(this->m_bufferData),this->getBuffLength());


Where is this being used? It looks like there is commented out code that we shouldn't deliver.

I was initially writing it as a string output to file to be able to read it. I've removed it since. I thought it would be nice to have for debugging purposes in the future.

Removed commented out code

timcanham · 2024-10-17T04:15:00Z

Fw/Trace/TraceEntity.hpp

+ * TraceEntity.hpp
+ *
+ *      Author: sreddy
+ */


Put in more comments describing what this class is for. Or, is it misspelled "TraceEntry"?

That's way better than TraceEntity. I'll change it.

timcanham · 2024-10-17T04:19:32Z

Fw/Trace/docs/sdd.md

+	- Event ID : A unique identifier recording a “Trace Event”
+	- Time Tag : Captures a high precision timestamp
+	- Client Data : A small sample of user arguments recorded into trace buffer
+3. Trace shall create trace points and log events in software automatically for thread switching(?) and  ...TBD, ask Tim


:) Tim says, and port calls in and out, and message queue and dequeue.

lol thanks Tim :)

timcanham · 2024-10-17T05:01:40Z

Svc/TraceFileLogger/TraceFileLogger.cpp

+
+            //Only log trace types that are enabled by either config or user
+            if(!(this->m_traceFilter & bit_mask)) {
+                //TODO: Should we generate an event here, letting user know that this specific filter is disabled?


No, it would spam events.

timcanham · 2024-10-17T05:06:02Z

Svc/TraceFileLogger/TraceFileLogger.cpp

+           //Make a call to reset
+           Fw::SerializeBufferBase& buf_ref = m_file_buffer.getSerializeRepr();
+           buf_ref.resetSer();
+           (void)buf_ref.serialize(id);


If these should never fail, you should assert OK.

timcanham · 2024-10-17T05:07:35Z

Svc/TraceFileLogger/TraceFileLogger.cpp

+           (void)buf_ref.serialize(timeTag);
+           if(FW_TRACE_RECORD_MINIMAL == false) {
+            (void)buf_ref.serialize(args);
+            traceSize = m_file_buffer.getSize(); //Record max size of each trace record for circular file


Not understanding this - why wouldn't it be getBuffLength() too?

Because if it is minimal, it is already fixed size and you can store more records...unless you think it would cause issues with ground decoding?

timcanham · 2024-10-17T05:08:21Z

Svc/TraceFileLogger/TraceFileLogger.cpp

+           //      ensure when the file is overwritten, we preserve older records
+           this->write_log_file(m_file_buffer.getData(),traceSize);
+           // If we choose not to use circular file write then use below instead. 
+           //this->write_log_file(m_file_buffer.getData(),buf_ref.getBuffLength());


Delete this; we can add it later if we want to do it.

timcanham · 2024-10-17T05:09:25Z

Svc/TraceFileLogger/TraceFileLogger.cpp

+    }
+
+    void TraceFileLogger ::DumpTraceDp_cmdHandler(FwOpcodeType opCode, U32 cmdSeq) {
+        this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::OK);


If we are writing a wrapping file, we don't need this?

I was thinking about that, it was part of the initial implementation, and I wasn't sure if we could configure the same file for both in-memory and file logger or if those two should be independent, given that there could be a redundancy.

Fw/Trace/TraceEntry.cpp

+    TraceEntry::~TraceEntry() {
+    }
+
+    SerializeStatus TraceEntry::serialize(SerializeBufferBase& buffer) const {


Fw/Trace/TraceEntry.cpp

+
+    }
+
+    SerializeStatus TraceEntry::deserialize(SerializeBufferBase& buffer) {


Svc/TraceFileLogger/ArrayProc.cpp

+        }
+    }
+
+    bool ArrayProc::search_array(U32 element, U8 *index) {


Shivaly-Reddy · 2024-10-17T20:15:38Z

config/TraceCfg.fpp

+        MAX_TRACE_TYPES = 9
+    }
+  }
+}


Confirm trace types 0 - 3 exists, do a compile time assert otherwise. Do this in Fw/

Shivaly-Reddy · 2024-10-17T20:17:27Z

Fw/Trace/TraceEntry.hpp

+
+namespace Fw {
+
+    class TraceEntry : public ComPacket {


Remove ComPacket , add serializable base class here instead

Shivaly-Reddy · 2024-10-17T20:20:57Z

Fw/Trace/TraceEntry.hpp

@@ -0,0 +1,41 @@
+/*


Remove Trace Entry entirely. Not used anywhere

Shivaly-Reddy · 2024-10-17T20:24:13Z

Svc/TraceFileLogger/TraceFileLogger.hpp

+#ifdef NAME_MAX
+#define FILE_NAME_MAX NAME_MAX
+#else
+#define FILE_NAME_MAX 255


delete this and use the macro in Config/FpConfig.h instead for file path and fine name

Shivaly-Reddy · 2024-10-17T20:25:21Z

Svc/TraceFileLogger/TraceFileLogger.hpp

+// Trace ID filter array size
+#ifndef FILTER_TRACE_ID_SIZE 
+#define FILTER_TRACE_ID_SIZE 10
+#endif


Put this in Config/TraceFileLoggerCfg.hpp

Shivaly-Reddy · 2024-10-17T20:27:37Z

Svc/TraceFileLogger/TraceFileLogger.hpp

+static const FwSizeType FW_TRACE_MAX_SER_SIZE = (FW_TRACE_BUFFER_MAX_SIZE + sizeof(FwTraceIdType) + Fw::Time::SERIALIZED_SIZE);
+
+//Mask bit for filtering on trace types
+static const U16 FILTER_BIT = 1;


Move it to *.cpp file and make it static const expr

Shivaly-Reddy · 2024-10-17T20:29:21Z

Svc/TraceFileLogger/TraceFileLogger.hpp

+    //!
+    //!  \param queueDepth the depth of the message queue for the component
+    //!  \param instance: instance identifier. Default: 0.
+    void  init(FwSizeType queueDepth, FwEnumStoreType instance = 0);


Delete this as its defined in the base class

Shivaly-Reddy · 2024-10-17T20:31:20Z

Svc/TraceFileLogger/TraceFileLogger.hpp

+    //!  \param maxSize The max size of the file
+    //!
+    //!  \return true if creating the file was successful, false otherwise
+    void set_log_file(const char* fileName, const U32 maxSize);


Put this in private section

Shivaly-Reddy · 2024-10-17T20:58:31Z

Svc/TraceFileLogger/TraceFileLogger.cpp

+
+        FW_ASSERT(fileName != nullptr);
+
+        if(this->m_enable_trace == true) {


On initialization, make sure the filename and size are stored and file is opened. When enable/disable command is sent, make sure the file writes continuously, on disable flush the file.

Shivaly-Reddy · 2024-10-31T20:16:46Z

Svc/TraceFileLogger/TraceFileLogger.cpp

+    m_maxFileSize(0),
+    m_byteCount(0),
+    m_log_init(false),
+    m_traceFilter(0)


TODO: Make an array proc constructor to pass the array reference and initialize the reference to the array here.

Shivaly-Reddy · 2024-10-31T20:24:00Z

Svc/TraceFileLogger/ArrayProc.hpp

+        //!  \param array_ptr pointer to the array to be used 
+        //!  \param array_size Max array index
+        //!  
+        void set_array(U32 *array_ptr, U8 array_size); 


Remove it and pass it as part of constructor as a reference

Shivaly-Reddy · 2024-10-31T20:29:32Z

Svc/TraceFileLogger/ArrayProc.hpp

+        //!  \param arrayIndex returns the index where (if) the array element exists
+        //!
+        //!  \return true if the element is found in the array, false otherwise
+        bool search_array(U32 element, U8 *index); 


Use FwSizeType instead of U8

Shivaly-Reddy and others added 30 commits March 27, 2024 18:37

2604: SDD in Progress

2a5dba3

2604: Fixed bug in version

2e67154

Merge branch 'nasa:devel' into devel

27a58bc

Merge branch 'nasa:devel' into devel

2156618

2604: Updates to user defined versions

25ede54

FP-2604: Updates to Version module and UTs

8d30c55

Merging from the forked

3638982

Merge branch 'devel' into FP-2604

6160ed2

FP-2604 : Updates based on changes made by Thomas to version.hpp

d1c48ca

FP-2604: Updates to Version based on conversations with Tim and Michael

5d50140

FP-2604: Updates to UTs and respective code changes

9b1cd59

Merge branch 'nasa:devel' into devel

3a5a366

Merge branch 'nasa:devel' into devel

ccf88a9

Merge branch 'nasa:devel' into devel

10ef05b

Revert "FP-2604: Updates to UTs and respective code changes"

3f0d2db

This reverts commit 9b1cd59.

Revert "FP-2604: Updates to Version based on conversations with Tim a…

1a000c2

…nd Michael" This reverts commit 5d50140.

Revert "FP-2604 : Updates based on changes made by Thomas to version.…

8c53be4

…hpp" This reverts commit d1c48ca.

Revert "FP-2604: Updates to Version module and UTs"

33dd922

This reverts commit 8d30c55.

Revert "2604: Updates to user defined versions"

11eabfe

This reverts commit 25ede54.

Revert "2604: Fixed bug in version"

9193a20

This reverts commit 2e67154.

Revert "2604: SDD in Progress"

aaadbab

This reverts commit 2a5dba3.

Making fork identical to the latest fprime devel branch

5d372d4

Merge branch 'nasa:devel' into devel

a4b5cb6

FP-2784 : Trace modules

b548ebb

Merge branch 'nasa:devel' into devel

b0a9ba0

FP-2674: Trace logger demo implementation (work in progress)

f99fc34

Merge branch 'devel' of https://github.com/Shivaly-Reddy/fprime into …

d598e4e

…devel

Merge branch 'devel' into FP-2784

74eb8f8

FP-2784: Trace devel

0f0692a

Merge branch 'FP-2784-temp' into FP-2784

ed63e2d

FP-2784: Updates based on bugs found during UTs and code coverage

2dbf993

Shivaly-Reddy requested review from timcanham and LeStarch October 15, 2024 19:10

github-advanced-security bot found potential problems Oct 15, 2024

View reviewed changes

Fw/Trace/TraceBuffer.hpp Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Oct 15, 2024

View reviewed changes

Shivaly-Reddy added 3 commits October 15, 2024 14:04

FP-2784: Fix trace references in Ref

b42e62c

FP-2784: Few uwpdates based on CodeQL warnings

9e10a49

FP-2784: Fix spell check errors

ca35e21

github-advanced-security bot found potential problems Oct 15, 2024

View reviewed changes

Shivaly-Reddy added 4 commits October 15, 2024 15:47

FP-2784: Deframer cmake file accidentally got altered

7539954

FP-2784: Try fixing CI error

7d4e9e4

FP-2784: Corrected codeql error

f3479d1

FP-2784: Cleanup

6bc9b92

timcanham requested changes Oct 17, 2024

View reviewed changes

Shivaly-Reddy added 2 commits October 17, 2024 12:18

FP-2784: Updates based on Tim's review comments

97af364

FP-2784: Oops forgot to add the modified files

179127e

github-advanced-security bot found potential problems Oct 17, 2024

View reviewed changes

Shivaly-Reddy commented Oct 17, 2024

View reviewed changes

Fw/Trace/TraceEntry.hpp

@@ -0,0 +1,41 @@

/*

Copy link

Collaborator Author

Shivaly-Reddy Oct 17, 2024

Choose a reason for hiding this comment

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

Remove Trace Entry entirely. Not used anywhere

Shivaly-Reddy commented Oct 17, 2024

View reviewed changes

Shivaly-Reddy commented Oct 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trace Logger Feature Review and Merge #2953

Trace Logger Feature Review and Merge #2953

Shivaly-Reddy commented Oct 15, 2024

github-advanced-security bot left a comment

timcanham left a comment

timcanham Oct 17, 2024

Shivaly-Reddy Oct 17, 2024

timcanham Oct 17, 2024

Shivaly-Reddy Oct 17, 2024

Shivaly-Reddy Oct 17, 2024

timcanham Oct 17, 2024

Shivaly-Reddy Oct 17, 2024

timcanham Oct 17, 2024

Shivaly-Reddy Oct 17, 2024

timcanham Oct 17, 2024

timcanham Oct 17, 2024

timcanham Oct 17, 2024

Shivaly-Reddy Oct 17, 2024

timcanham Oct 17, 2024

timcanham Oct 17, 2024

Shivaly-Reddy Oct 17, 2024 •

edited

Loading

Shivaly-Reddy Oct 17, 2024

Shivaly-Reddy Oct 17, 2024

Shivaly-Reddy Oct 17, 2024

Shivaly-Reddy Oct 17, 2024

Shivaly-Reddy Oct 17, 2024 •

edited

Loading

Shivaly-Reddy Oct 17, 2024 •

edited

Loading

Shivaly-Reddy Oct 17, 2024

Shivaly-Reddy Oct 17, 2024

Shivaly-Reddy Oct 17, 2024

Shivaly-Reddy Oct 31, 2024

Shivaly-Reddy Oct 31, 2024

Shivaly-Reddy Oct 31, 2024


		}

		SerializeStatus TraceEntry::deserialize(SerializeBufferBase& buffer) {


		FW_ASSERT(fileName != nullptr);

		if(this->m_enable_trace == true) {

Trace Logger Feature Review and Merge #2953

Are you sure you want to change the base?

Trace Logger Feature Review and Merge #2953

Conversation

Shivaly-Reddy commented Oct 15, 2024

Change Description

Rationale

Testing/Review Recommendations

Future Work

github-advanced-security bot left a comment

Choose a reason for hiding this comment

timcanham 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

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

Shivaly-Reddy Oct 17, 2024 • 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

Shivaly-Reddy Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Shivaly-Reddy Oct 17, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shivaly-Reddy Oct 17, 2024 •

edited

Loading

Shivaly-Reddy Oct 17, 2024 •

edited

Loading

Shivaly-Reddy Oct 17, 2024 •

edited

Loading