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

Trace Logger Feature Review and Merge #2953

Open
wants to merge 52 commits into
base: devel
Choose a base branch
from

Conversation

Shivaly-Reddy
Copy link
Collaborator

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.

Shivaly-Reddy and others added 30 commits March 27, 2024 18:37
TraceBuffer::~TraceBuffer() {
}

TraceBuffer::TraceBuffer(const TraceBuffer& other) : Fw::SerializeBufferBase() {

Check warning

Code scanning / CppCheck

Member variable 'TraceBuffer::m_bufferData' is not assigned in the copy constructor. Should it be copied? Warning

Member variable 'TraceBuffer::m_bufferData' is not assigned in the copy constructor. Should it be copied?
Fw/Trace/TraceBuffer.hpp Fixed Show fixed Hide fixed
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

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

Fw/Trace/TraceEntity.cpp Fixed Show fixed Hide fixed
Fw/Trace/TraceEntity.cpp Fixed Show fixed Hide fixed
return false;
}

bool ArrayProc::delete_element(U32 element) {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
Svc/TraceFileLogger/ArrayProc.cpp Fixed Show fixed Hide fixed
// Component construction and destruction
// ----------------------------------------------------------------------

TraceFileLogger::TraceFileLogger(const char* const compName) :

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
TraceFileLoggerComponentBase::init(queueDepth,instance);
}

void TraceFileLogger::set_log_file(const char* fileName, const U32 maxSize) {

Check notice

Code scanning / CodeQL

Long function without assertion Note

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

// 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){

Check notice

Code scanning / CodeQL

Long function without assertion Note

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

}

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

Check notice

Code scanning / CodeQL

Long function without assertion Note

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

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

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
// Handler implementations for user-defined typed input ports
// ----------------------------------------------------------------------

void TraceFileLogger::TraceBufferLogger_handler(FwIndexType portNum,

Check notice

Code scanning / CodeQL

Long function without assertion Note

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

@timcanham timcanham left a comment

Choose a reason for hiding this comment

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

Some changes...

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was part of initial implementation . Removed it.


TraceBuffer::TraceBuffer(const U8 *args, NATIVE_UINT_TYPE size) {
SerializeStatus stat = SerializeBufferBase::setBuff(args,size);
FW_ASSERT(FW_SERIALIZE_OK == stat, static_cast<FwAssertArgType>(stat));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The warning seems right - should we add an assert for args?

//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());
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed commented out code

* TraceEntity.hpp
*
* Author: sreddy
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

- 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lol thanks Tim :)


//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?
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it would spam events.

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

Choose a reason for hiding this comment

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

If these should never fail, you should assert OK.

(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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

// 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());
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@Shivaly-Reddy 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.

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.

TraceEntry::~TraceEntry() {
}

SerializeStatus TraceEntry::serialize(SerializeBufferBase& buffer) const {

Check notice

Code scanning / CodeQL

Long function without assertion Note

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

}

SerializeStatus TraceEntry::deserialize(SerializeBufferBase& buffer) {

Check notice

Code scanning / CodeQL

Long function without assertion Note

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

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

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
MAX_TRACE_TYPES = 9
}
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


namespace Fw {

class TraceEntry : public ComPacket {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove ComPacket , add serializable base class here instead

@@ -0,0 +1,41 @@
/*
Copy link
Collaborator Author

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

#ifdef NAME_MAX
#define FILE_NAME_MAX NAME_MAX
#else
#define FILE_NAME_MAX 255
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

// Trace ID filter array size
#ifndef FILTER_TRACE_ID_SIZE
#define FILTER_TRACE_ID_SIZE 10
#endif
Copy link
Collaborator Author

@Shivaly-Reddy 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.

Put this in Config/TraceFileLoggerCfg.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;
Copy link
Collaborator Author

@Shivaly-Reddy 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.

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

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

Choose a reason for hiding this comment

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

Delete this as its defined in the base class

//! \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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Put this in private section


FW_ASSERT(fileName != nullptr);

if(this->m_enable_trace == true) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants