Implement Cell Data Logging with Timestamping #176

KSMehta11 · 2025-02-11T20:23:44Z

Changes

Added cell data logging functionality using a ring buffer.
Implemented timestamping for cell data using TIM2 in microseconds.
Stored the last N measurements in a static array.
Provided functions for retrieving data: last log, last n logs.
Implemented functions to view logs on serial terminal.

Notes

The buffer size can be adjusted in the header file to store more readings.

Test Cases

Test A: Fake BMS Data on STM32F4 Nucleo
The logging functionality was first tested on an STM32 Nucleo board using fake BMS data. Simulated cell voltages and temperatures were logged, retrieved, and verified using serial printouts.
Test B: Real BMS Data on Shepherd BMS Master Board
The logging functionality was tested on the Shepherd BMS Master Board, configured with one chip Alpha. The inputs were connected to a PSU to simulate real cell voltages.

To Do

Validate logging behavior with real BMS data instead of test cases.
Validate functionality with additional chips and segments.

Checklist

It can be helpful to check the Checks and Files changed tabs.
Please reach out to your Project Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.

No merge conflicts
All checks passing
Remove any non-applicable sections of this template
Assign the PR to yourself
Request reviewers & ping on Slack
PR is linked to the ticket (fill in the closes line below)

Closes # (issue #175 )

Sabramz · 2025-02-12T00:52:44Z

Core/Src/cell_data_logging.c

+
+void cell_data_logger_init(void)
+{
+	rb_init(&cell_log_ring_buff, cell_data_storage, NUM_OF_READINGS,


Can all of this be wrapped in a ptr that the user controls? So the arg to this func (and all other funcs) would be like struct *bms_logger. If you don't want them modifying it, you can use C stuff to make a struct that is defined in the header but its fields are defined in the source file and the fields cannot be accessed from outside the source.

Also this struct needs a mutex, and all the getters and setters will need mutexes as well.

Yes, I can wrap this in a pointer-controlled struct, and that approach makes sense. I will do it that way and add the mutex as well.

Sabramz · 2025-02-12T00:54:40Z

Core/Src/cell_data_logging.c

+			new_entry.cell_temperatures[chip_num][cell] =
+				bms_data->chip_data[chip_num].cell_temp[cell];
+
+			new_entry.timestamp[chip_num][cell] =


Can the source of this timestamp be right after the measurement is taken, rather than when it is logged?

We may want to log the data much later than the reading happened.

Core/Src/cell_data_logging.c

Sabramz · 2025-02-12T00:58:05Z

Core/Src/cell_data_logging.c

+
+CellDataEntry_t *cell_data_log_get_last(void)
+{
+	return (CellDataEntry_t *)rb_get_head(&cell_log_ring_buff);


Can you protect against getting a NULLPTR from this? or say in docs that NULL PTR can be returned and should be checked and that a NULL ptr return means the rb has not been initted / populated with data yet?

I will add protection against returning a nullptr and will likely include a message.

…hem' of https://github.com/Northeastern-Electric-Racing/Shepherd-BMS into 166-24a---storing-cell-data-readings-and-timestamping-them

Sabramz · 2025-02-16T16:16:10Z

Core/Src/cell_data_logging.c

+	osMutexId_t mutex;
+};
+
+static BMSLogger bms_logger = { .is_initialized = false };


We don't want this. The user should be creating the logger. They will make the struct, and then pass it to an init function.

Sabramz · 2025-02-16T16:16:21Z

Core/Src/cell_data_logging.c

+
+static BMSLogger bms_logger = { .is_initialized = false };
+
+BMSLogger *get_logger(void)


Get rid of this with the static global variable

Sabramz · 2025-02-16T16:19:21Z

Core/Inc/cell_data_logging.h

+ * @param logger Pointer to the logger instance.
+ * @return false on success, true on failure.
+ */
+bool cell_data_logger_init(BMSLogger *logger);


The standard in C is that the return type is an int. Make every error code return an int.

Sabramz · 2025-02-16T16:21:39Z

Core/Src/cell_data_logging.c

+extern TIM_HandleTypeDef htim2;
+
+struct BMSLogger {
+	bool is_initialized;


This is unnecessary. You will know if the logger is uninitialized if a NULLPTR is passed into a func in the interface.

Sabramz · 2025-02-16T16:23:06Z

Core/Src/cell_data_logging.c

+	return __HAL_TIM_GET_COUNTER(&htim2);
+}
+
+static bool get_logger_status(const BMSLogger *logger)


Add doc comment and put private functions before all public functions.

Since you are also checking if acc_data_t is null in other functions, make this function able to determine if any pointer is a nullptr, not just loggers.

Tbh you can just get rid of this entirely and use asserts. Just at the beginning of every function call assert(ptr) for every ptr.

Sabramz · 2025-02-16T16:31:53Z

Core/Inc/cell_data_logging.h

+#include "cmsis_os.h"
+
+// Number of stored cell data readings in ring buffer.
+#define NUM_OF_READINGS 10


The user should decide how many readings will be logged, and should be able to do this when initializing the logger.

Core/Src/cell_data_logging.c

Sabramz · 2025-02-16T16:35:28Z

Core/Inc/cell_data_logging.h

+ * @brief Serial prints the most recent data log.
+ * @param logger Pointer to the logger instance.
+ */
+void print_latest_cell_data_log(const BMSLogger *logger);


This function is not needed since the user can just call print last n cell data logs with an n of 1.

Sabramz · 2025-02-16T16:36:10Z

Core/Src/cell_data_logging.c

+		       log_entries[entry_idx].cell_temperature_timestamp);
+
+		for (int chip_num = 0; chip_num < NUM_CHIPS; chip_num++) {
+			int cell_count = (chip_num % 2 == 0) ? NUM_CELLS_ALPHA :


This should be broken out into a helper function. Triple nested for loops is going too far lolz.

Sabramz · 2025-02-16T16:37:07Z

Core/Inc/datastructs.h

@@ -142,6 +142,10 @@ typedef struct {

 	bool is_charger_connected;

+	/* Cell Data Logging Timestamps */
+	uint32_t voltage_timestamp;


My mistake on the previous review. Lets limit the scope of this to just the interface. Application will come later, so get rid of these. Everything should be in cell data logging.

Sabramz · 2025-02-20T17:20:46Z

Core/Src/cell_data_logging.c

+ * @brief Assigns a timestamp to the voltage measurement field of a cell data entry.
+ * @param entry Pointer to the CellDataEntry_t structure to update.
+ */
+void cell_data_set_voltage_timestamp(CellDataEntry_t *entry)


When we timestamp something, we should just timestamp the next log entry, cuz that is where the next reading will be written to. Do some auto incrementing here so the user simply calls timestamp_next_reading and passes in the logger.

Sabramz · 2025-02-20T22:43:36Z

Core/Inc/cell_data_logging.h

+ * @param entry Pointer to a CellDataEntry_t structure that holds the measurement data.
+ * @return 0 on success, -1 on failure.
+ */
+int cell_data_log_measurement(struct BMSLogger *logger, acc_data_t *bms_data,


Make this not take an entry as an argument. Just a logger and data to insert.

Kaushik Mehta added 4 commits February 10, 2025 18:57

Added cell data logging header with function prototypes.

001d630

Added cell data logger initialization and updated Makefile.

4efa3ac

Implemented function to log cell voltage and temperature data.

932fcb8

Implemented functions to retrieve logged cell data.

81838cd

KSMehta11 self-assigned this Feb 11, 2025

KSMehta11 linked an issue Feb 11, 2025 that may be closed by this pull request

24A - Storing Cell Data Readings and Timestamping Them #166

Open

Implemented serial debugging functions.

2ddecdc

KSMehta11 requested a review from Sabramz February 11, 2025 21:49

Initialized the data logging ring buffer and enabled logging.

5202ac7

Sabramz requested changes Feb 12, 2025

View reviewed changes

Kaushik Mehta added 10 commits February 13, 2025 19:00

Feature: Add BMSLogger struct.

d7c9364

Refactor: Encapsulate BMSLogger

e311d8d

Add separate timestamps for voltage and temperature measurements.

6e58239

Feat: Capture timestamps at measurement instead of storage.

84adf2a

Add error messages for user diagnostics.

36117e1

Fix: prevent multiple initializations of BMSLogger.

30e9c2b

Add Mutex and Error Handling.

01f4922

Refactor: Mutex wait time.

01d0110

Refactor: Mutex

70a66a1

Merge branch '166-24a---storing-cell-data-readings-and-timestamping-t…

d7e576f

…hem' of https://github.com/Northeastern-Electric-Racing/Shepherd-BMS into 166-24a---storing-cell-data-readings-and-timestamping-them

KSMehta11 requested a review from Sabramz February 16, 2025 02:50

Sabramz requested changes Feb 16, 2025

View reviewed changes

Kaushik Mehta added 8 commits February 16, 2025 12:04

Refactor: Remove static global logger and use getLogger.

05d5e7b

Refactor: Use int return type and remove is_initialized flag.

04e7ba5

Refactor: Replace manual NULL checks with assert.

7cfa7bb

Implement single return point using goto.

6b656cc

Refactor: Mutex wait time.

dcd5678

Add print_cell_data helper func.

30f2846

Add Doc Comments

2010a5d

Add global bms logger instance.

ead1d0a

Kaushik Mehta added 3 commits February 16, 2025 18:46

Remove: Timestamp during measurement.

da715db

Refactor: Made bms logger public.

0822ce1

Add: Timestamp set functions.

eb059c8

KSMehta11 requested a review from Sabramz February 17, 2025 21:07

Refactor: CellDataEntry struct.

fc12135

Sabramz requested changes Feb 20, 2025

View reviewed changes

Refactored: Data Insertion and Timestamping Functions.

6f7ae82

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Cell Data Logging with Timestamping #176

Implement Cell Data Logging with Timestamping #176

KSMehta11 commented Feb 11, 2025 •

edited

Loading

Sabramz Feb 12, 2025

Sabramz Feb 12, 2025

KSMehta11 Feb 12, 2025

Sabramz Feb 12, 2025

Sabramz Feb 12, 2025

Sabramz Feb 12, 2025

KSMehta11 Feb 12, 2025

Sabramz Feb 16, 2025

Sabramz Feb 16, 2025

Sabramz Feb 16, 2025

Sabramz Feb 16, 2025

Sabramz Feb 16, 2025

Sabramz Feb 16, 2025

Sabramz Feb 16, 2025

Sabramz Feb 16, 2025

Sabramz Feb 16, 2025

Sabramz Feb 16, 2025

Sabramz Feb 16, 2025

Sabramz Feb 20, 2025

Sabramz Feb 20, 2025


		static BMSLogger bms_logger = { .is_initialized = false };

		BMSLogger *get_logger(void)

Implement Cell Data Logging with Timestamping #176

Are you sure you want to change the base?

Implement Cell Data Logging with Timestamping #176

Conversation

KSMehta11 commented Feb 11, 2025 • edited Loading

Changes

Notes

Test Cases

To Do

Checklist

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

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

KSMehta11 commented Feb 11, 2025 •

edited

Loading