Logging last 30 seconds of data using file ping pong buffer #13
Conversation
… of logging data as well as not overwriting existing data by checking file names. A lot of changes were added as I did a lot of work before commiting.
…dded timespec_diff function to find difference in time between two timespec structs. I am using clock_gettime() to get the current system time to compare to time of the last modification.
…the extraction file. Changed the way flight and extraction log file names are constructed. Fixed some typos in variable names. Added teardown function.
…ve logic in the IDLE state that is also done in the AIRBORNE state. Will remove it if not needed
…ting file descriptor for active storage file before calling fstat on it. Changed some variable names for better readability and commented out some code."
|
There are a few things left to do or check, indicated by TODOs in the code. I also think error handling could be improved a bit, right now it's mostly just printing if the debug option is defined. |
|
Can you resolve the merge conflicts and work with @AngusJull to test this on Josh? |
telemetry/src/logging/logging.c
Outdated
| } | ||
| } | ||
|
|
||
| int switch_active_log_file(FILE **active_storage_file, FILE **storage_file_1, FILE **storage_file_2, char **flight_filename2) |
There was a problem hiding this comment.
I want to guess there's a way to work in a more general way to write this func where you just pass active_storage_file and alternate_storage_file (or whatever name you want). Then if the alternate storage file is NULL open it. Then if we ever want to change how the rest of our code works this has a better chance of still working as needed
|
Looking good! Logger is getting pretty big now so if you see blocks of code with a singular purpose you could potentially make them into functions. Like if we factored things out enough, the high-level overview would kind of be spelled out through the names of the functions we're calling in logger's body. We can give it a try on Josh, but I forget if you had the simulator set up. If not, we could try running the code on that and see if the buffers fill up and overwrites happen as expected. We can even use GDB to check when the ping pong buffers get switched to verify it happens when we expect it to. So far in my testing I've found the simulator to be accurate to NuttX on Josh so if it works on there, we can be confident it'll do the same on the real system. |
…. Removed pseudocode, some redundant commented code, and teardown function as it wasn't really needed."
…ndling messages, comments, and formatting. Also fixed some typos.
…double pointer to fix how files were switched. Put testing print code in debug conditionals. Also, removed some testing code.
… checks if in debug to print values.
…light filenames and extraction filenames.
| written = fwrite(&state->data, sizeof(state->data), 1, active_storage_file); | ||
| #if defined(CONFIG_INSPACE_TELEMETRY_DEBUG) | ||
| printf("Logged %u bytes\n", written * sizeof(state->data)); | ||
| #endif /* defined(CONFIG_INSPACE_TELEMETRY_DEBUG) */ | ||
| DEBUG_FPRINTF(stdout, "Logged %lu bytes\n", written * sizeof(state->data)); | ||
|
|
There was a problem hiding this comment.
Not 100% familiar with the changes that were made to sharing data between threads, but haven't you already logged the packet a few lines earlier with log_packet? Do you need to log the state data then if that's not how packets are being shared anymore?
| } | ||
|
|
||
|
|
||
| /* If we are in the idle state, only write the latest n seconds of data*/ | ||
| if (flight_state == STATE_IDLE) | ||
| { |
There was a problem hiding this comment.
Write a note here that this condition is because of the switch case fall-through.
| if (err) | ||
| { | ||
| /* Handle Error */ | ||
| DEBUG_FPRINTF(stderr, "Couldn't seek active file back to start: %d", err); | ||
| } | ||
|
|
||
| /* Copy from one to the other using a buffer */ |
There was a problem hiding this comment.
Nitpick but you could make the file copying its own helper function.
| /* Close files that may be open */ | ||
| if (fclose(storage_file_1) != 0) | ||
| { | ||
| err = errno; | ||
| DEBUG_FPRINTF(stderr, "Failed to close flight logfile 1 handle: %d\n", err); | ||
| } | ||
|
|
||
| if (fclose(storage_file_2) != 0) | ||
| { | ||
| err = errno; | ||
| DEBUG_FPRINTF(stderr, "Failed to close flight logfile 2 handle: %d\n", err); | ||
| } | ||
|
|
There was a problem hiding this comment.
Could maybe make this a cleanup function so you don't need to write this out every time you exit?
| #endif /* defined(CONFIG_INSPACE_TELEMETRY_DEBUG) */ | ||
|
|
||
| /* Set Base time to current time to reset*/ | ||
| base_timespec = new_timespec; | ||
| } |
There was a problem hiding this comment.
Is this meant to fall through to the airborne case to log data here?
| /* Infinite loop to log data */ | ||
|
|
||
| /* Wait for the data to have a change */ | ||
|
|
||
| err = state_wait_for_change(state, &version); |
There was a problem hiding this comment.
It appears you're waiting for a change to log data in the idle case only. You should be waiting for a change to log anything, including in the airborne case. If you fix that, it means that because of the fall-through you'll wait twice in IDLE and AIRBORNE states. Make it so the idle state doesn't wait, the airborne state does (right before logging whatever new data it gets).
Summary
Idle/AirborneState
Landed State
Idle