Logging for Interface by panthpatel2016 · Pull Request #328 · UWOrbital/OBC-firmware

panthpatel2016 · 2025-03-09T20:04:14Z

Purpose

Implemented Logging for Interface.

New Changes

Created a logging module called obc_gs_logging.h that routes logging calls based on CMAKE_BUILD_TYPE.
Updated the CMakeLists.txt for the obc-gs-interface target to inject compile definitions based on the CMAKE_BUILD_TYPE.

Testing

Testing incomplete

Outstanding Changes

none

…to add-logging-task

github-actions · 2025-03-09T20:04:29Z

Pull reviewers stats

Stats of the last 120 days for UWOrbital:

	User	Total reviews	Time to review	Total comments
	kepler452b123	20	2d 4h 15m	87
	Yarik-Popov	12	1d 3h 24m	110

^{⚡️ Pull request stats}

Yarik-Popov

There's a few issues with the PR:

You should be generalizing obc_logging.h, not creating a new set of macros for logging in obc gs. Have a look at #277 for a starting point
The obc_gs_logging.h isnt used anywhere and as such the build succeed even if there are functions that arent implemented. You will need to update all the .c files in the interfaces directory to point to your new logging.h file once u generalize it. Possibly updating all .c filrs that use obc_logging.h to point to the new generalize solution

interfaces/CMakeLists.txt

interfaces/obc_gs_interface/common/obc_gs_logging.h

Yarik-Popov

I've added some comments to help clarify how to implement this.

Yarik-Popov · 2025-03-10T17:13:49Z

obc/app/sys/logging/obc_logging.h

You will need to generalize this file and move most of it to interfaces

Yarik-Popov · 2025-03-10T23:01:09Z

interfaces/CMakeLists.txt

+if(CMAKE_BUILD_TYPE MATCHES OBC)
+    target_compile_definitions(${OBC_GS_INTERFACE_LIB_NAME} PUBLIC BUILD_TYPE_OBC_FW)
+
+elseif(CMAKE_BUILD_TYPE MATCHES GS)
+    target_compile_definitions(${OBC_GS_INTERFACE_LIB_NAME} PUBLIC BUILD_TYPE_OBC_GS)
+
+endif()


You dont need this code. You will need to conditionally add the source (.c) file that implements logMsg and logErrorCode. If the build type is obc, it will use the obc implementation at also implements the isr versions.

If its gs, it will add the source file that you will add in the interfaces/common directory.

Otherwise, you will add the source file that implements stubs for these 2 functions. Basically it return success in this last case and ignore all arguments

Yarik-Popov · 2025-03-10T23:05:30Z

obc/app/sys/logging/obc_logging.h

 #define LOG_DEFAULT_LEVEL LOG_TRACE
 #endif

+#ifdef BUILD_TYPE_OBC_FW


Move the lines 48-53 and 62 and 65-103 below to a new logging.h file located in the interfaces/common directory. You will need to change the OBC_ERR_CODE_SUCCESS to 0 as success should always be 0. You should add a static assert for this in the obc_logging.c file. Also move the lines 10-11 and 25-44 to that new interfaces logging.h file.

Make the current file include that new logging.h file.

You will also need to update the CmakeLists.txt to have the point to that this logging.h header file in the interfaces/common directory

Add new logging error codes that are will be put inside of the logging.h file

Update the logging implementation files on the obc to return the new logging error codes

Yarik-Popov · 2025-03-10T23:07:54Z

obc/app/sys/logging/obc_logging.h

+
+#else
+
+#ifdef BUILD_TYPE_OBC_GS
+
+#define LOG_TRACE(msg) gsLogMsg(LOG_TRACE, __FILE_FROM_REPO_ROOT__, __LINE__, msg)
+#define LOG_DEBUG(msg) gsLogMsg(LOG_DEBUG, __FILE_FROM_REPO_ROOT__, __LINE__, msg)
+#define LOG_INFO(msg) gsLogMsg(LOG_INFO, __FILE_FROM_REPO_ROOT__, __LINE__, msg)
+#define LOG_WARN(msg) gsLogMsg(LOG_WARN, __FILE_FROM_REPO_ROOT__, __LINE__, msg)
+#define LOG_ERROR(msg) gsLogMsg(LOG_ERROR, __FILE_FROM_REPO_ROOT__, __LINE__, msg)
+#define LOG_FATAL(msg) gsLogMsg(LOG_FATAL, __FILE_FROM_REPO_ROOT__, __LINE__, msg)
+
+#define LOG_TRACE_FROM_ISR(msg) LOG_TRACE(msg)
+#define LOG_DEBUG_FROM_ISR(msg) LOG_DEBUG(msg)
+#define LOG_INFO_FROM_ISR(msg) LOG_INFO(msg)
+#define LOG_WARN_FROM_ISR(msg) LOG_WARN(msg)
+#define LOG_ERROR_FROM_ISR(msg) LOG_ERROR(msg)
+#define LOG_FATAL_FROM_ISR(msg) LOG_FATAL(msg)
+
+#define LOG_ERROR_CODE(errCode) gsLogErrorCode(LOG_ERROR, __FILE_FROM_REPO_ROOT__, __LINE__, errCode)
+#define LOG_ERROR_CODE_FROM_ISR(errCode) gsLogErrorCode(LOG_ERROR, __FILE_FROM_REPO_ROOT__, __LINE__, errCode)
+
+#define RETURN_IF_ERROR_CODE(_ret)            \
+  do {                                        \
+    errCode = _ret;                           \
+    if (errCode != OBC_GS_ERR_CODE_SUCCESS) { \
+      LOG_ERROR_CODE(errCode);                \
+      return errCode;                         \
+    }                                         \
+  } while (0)
+
+#define LOG_IF_ERROR_CODE(_ret)               \
+  do {                                        \
+    errCode = _ret;                           \
+    if (errCode != OBC_GS_ERR_CODE_SUCCESS) { \
+      LOG_ERROR_CODE(errCode);                \
+    }                                         \
+  } while (0)
+
+  /**
+ * @brief Log an error code
+ *
+ * @param msgLevel Level of the message
+ * @param file File of message
+ * @param line Line of message
+ * @param errCode the error code that needs to be logged
+ * @return obc_gs_error_code_t OBC_GS_ERR_CODE_LOG_MSG_SILENCED if msgLevel is lower than logging level
+ *                              OBC_GS_ERR_CODE_BUFF_TOO_SMALL if logged message is too long
+ *                              OBC_GS_ERR_CODE_INVALID_ARG if file or s are null or if there is an encoding error
+ *                              OBC_GS_ERR_CODE_SUCCESS if message is successfully logged
+ *                              OBC_GS_ERR_CODE_UNKNOWN otherwise
+ */
+obc_gs_error_code_t gsLogErrorCode(gs_log_level_t msgLevel, const char *file, uint32_t line, uint32_t errCode);
+
+/**
+ * @brief Log a message
+ *
+ * @param msgLevel Level of the message
+ * @param file File of message
+ * @param line Line of message
+ * @param msg the message that should be logged (MUST BE STATIC)
+ * @return obc_gs_error_code_t OBC_GS_ERR_CODE_LOG_MSG_SILENCED if msgLevel is lower than logging level
+ *                              OBC_GS_ERR_CODE_BUFF_TOO_SMALL if logged message is too long
+ *                              OBC_GS_ERR_CODE_INVALID_ARG if file or s are null or if there is an encoding error
+ *                              OBC_GS_ERR_CODE_SUCCESS if message is successfully logged
+ *                              OBC_GS_ERR_CODE_UNKNOWN otherwise
+ */
+obc_gs_error_code_t gsLogMsg(gs_log_level_t msgLevel, const char *file, uint32_t line, const char *msg);
+
+  #else
+
+  #define LOG_DEBUG(msg) ((void)0)
+  #define LOG_INFO(msg) ((void)0)
+  #define LOG_WARN(msg) ((void)0)
+  #define LOG_ERROR(msg) ((void)0)
+  #define LOG_FATAL(msg) ((void)0)
+  #define LOG_TRACE_FROM_ISR(msg) ((void)0)
+  #define LOG_DEBUG_FROM_ISR(msg) ((void)0)
+  #define LOG_INFO_FROM_ISR(msg) ((void)0)
+  #define LOG_WARN_FROM_ISR(msg) ((void)0)
+  #define LOG_ERROR_FROM_ISR(msg) ((void)0)
+  #define LOG_FATAL_FROM_ISR(msg) ((void)0)
+  #define LOG_ERROR_CODE(errCode) ((void)0)
+  #define LOG_ERROR_CODE_FROM_ISR(errCode) ((void)0)
+
+  #define RETURN_IF_ERROR_CODE(_ret)            \
+    do {                                        \
+      (void)(_ret);                             \
+      return _ret;                              \
+    } while (0)
+
+  #define LOG_IF_ERROR_CODE(_ret)               \
+    do {                                        \
+      (void)(_ret);                             \
+    } while (0)
+  #endif
+
+  #endif


This code isn't needed as we are going to be changing the implementation (.c file) in the cmakelist.txt file to handle the different logging environments

Yarik-Popov · 2025-03-10T23:11:43Z

interfaces/obc_gs_interface/aes128/obc_gs_aes128.c

@@ -1,5 +1,6 @@
 #include "obc_gs_aes128.h"
 #include "obc_gs_errors.h"
+#include "obc_logging.h"


Interfaces can't depend on obc. It should be the other way around

Yarik-Popov · 2025-03-10T23:12:00Z

interfaces/CMakeLists.txt

+    ${CMAKE_CURRENT_SOURCE_DIR}/../obc/app/sys/logging
+    ${CMAKE_CURRENT_SOURCE_DIR}/../obc/app/sys


Remove these. See the comments below

panthpatel2016 added 8 commits March 6, 2025 23:37

first test commit

09c9484

this commit completes the basis of the task

5396716

made small changes

f2025c5

small changes

c6bf9a5

final commit before PR

99eb19d

implemented logging

4837841

Merge remote-tracking branch 'origin/panth/add-condtional-logging' in…

1385040

…to add-logging-task

test commit

b51dca5

panthpatel2016 added 3 commits March 9, 2025 16:24

fixed formatting for PR

36dad41

indentation changes

0a7f05f

final commit

e856f63

Yarik-Popov assigned panthpatel2016 Mar 9, 2025

Yarik-Popov requested review from Navtajh04 and kepler452b123 March 9, 2025 21:09

Yarik-Popov requested changes Mar 9, 2025

View reviewed changes

panthpatel2016 added 6 commits March 9, 2025 20:57

fixed some comments

13ef2b1

generalised logging for both GS and FW

252f2d8

added obc_logging.h in cmakelists.txt for interfaces

bd744ad

fixed erorr in file path

6d03258

fixed duplicate files

839b417

small changes to complier definitions

376d8ba

Yarik-Popov requested changes Mar 10, 2025

View reviewed changes

Merge branch 'main' into panth/add-condtional-logging

ef50fe2

Yarik-Popov linked an issue Jun 4, 2025 that may be closed by this pull request

Implement logging for interfaces #489

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logging for Interface#328

Logging for Interface#328
panthpatel2016 wants to merge 18 commits intomainfrom
panth/add-condtional-logging

panthpatel2016 commented Mar 9, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Mar 9, 2025

Uh oh!

Yarik-Popov left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yarik-Popov left a comment

Uh oh!

Yarik-Popov Mar 10, 2025

Uh oh!

Yarik-Popov Mar 10, 2025

Uh oh!

Yarik-Popov Mar 10, 2025

Uh oh!

Yarik-Popov Mar 10, 2025

Uh oh!

Yarik-Popov Mar 10, 2025

Uh oh!

Yarik-Popov Mar 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		${CMAKE_CURRENT_SOURCE_DIR}/../obc/app/sys/logging
		${CMAKE_CURRENT_SOURCE_DIR}/../obc/app/sys

Conversation

panthpatel2016 commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

New Changes

Testing

Outstanding Changes

Uh oh!

github-actions bot commented Mar 9, 2025

Pull reviewers stats

Uh oh!

Yarik-Popov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yarik-Popov left a comment

Choose a reason for hiding this comment

Uh oh!

Yarik-Popov Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

Yarik-Popov Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

Yarik-Popov Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

Yarik-Popov Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

Yarik-Popov Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

Yarik-Popov Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

panthpatel2016 commented Mar 9, 2025 •

edited

Loading