Protected Sensor Data Reads#95
Conversation
ETSells
left a comment
ETSells
left a comment
There was a problem hiding this comment.
Functionally, this looks right. However, there are quite a few standards issues I'm seeing, so I would like them fixed before re-reviewing.
|
Everything you pointed out should be fixed now! |
|
my god, man... do you sleep?? will take a look today, thanks for the quick turnaround lol |
There was a problem hiding this comment.
Functionally looks good. I had some whitespace nitpicks that I went in and fixed myself. If you're curious about what these were, feel free to look. I also fixed one of my previous mistakes that I saw just under your work (failed to properly attribute a section of sensor.c to Bosch).
My big recommendation is to check your tabs vs spaces. Right now, it looks like your tabs are set up to introduce 8 spaces. Most other text editors (especially for C-style code syntax) default to 4.
Recommend holding merge pending a manual integration test on "Bad Drogue" FC.
| * Returns the baro_data_ready flag * | ||
| * * | ||
| *******************************************************************************/ | ||
| bool baro_get_baro_data_ready |
There was a problem hiding this comment.
Fixing this myself bc it's a total nitpick but whitespace is still a little off. it should be two lines between functions
There was a problem hiding this comment.
You appear to be double indenting (8 spaces instead of 4) when you hit tab. This is likely a setting in vim that can be updated.
There was a problem hiding this comment.
Also if you have a "tabs -> spaces" setting, I recommend enabling that as well. Tabs can be variable between different text editors, but if it converts to spaces then it'll render correctly across them.
| * * | ||
| * DESCRIPTION: * | ||
| * Returns the imu_data_ready flag * | ||
| * * |
There was a problem hiding this comment.
nit: Only one line of spacing at the bottom of a procedure header
| return baro_ready | imu_ready; | ||
| } | ||
| /* Ensure both the IMU and barometer are ready to be read */ | ||
| if ( imu_get_imu_data_ready() && imu_get_mag_data_ready() && baro_get_baro_data_ready() ) |
There was a problem hiding this comment.
My recommendation for compound conditions like this is to separate out to multiple lines if there are any more than 2 components. Tends to make it more readable.
Ex:
/* Ensure both the IMU and barometer are ready to be read */
if ( imu_get_imu_data_ready()
&& imu_get_mag_data_ready()
&& baro_get_baro_data_ready() )
{
return SENSOR_OK;
}
Note the alignment of the '&&' with the opening parentheses so that each component starts on the same column.
There was a problem hiding this comment.
We can remove this since the next block is under the same cond-def
|
Gahh, I thought I them all. I wrote down some of the formatting conventions I've seen thus far so hopefully I'll be better about that in the future lol. You're totally right about the 8-indent tabs, though. I have tab -> spaces on, but sometimes there were secret hidden tabs which threw off my formatting in some cases. I'll make changes to my config. Is the integration testing something we'll go over on Friday? |
|
Yeah no worries lol! Wanted to save us from doing like 4 rounds of revisions on an otherwise spotless PR haha. The integration test for this basically just needs to verify that we're still getting sensor data. Don't worry about checking the functional requirement (preventing race conditions in modifications) and instead focus on making sure it doesn't crash & all of the sensor data still exists. We can run this on bad drogue w/ the debugger. Alternatively, we might have an FC by friday morning, so we could also go over it then. |
|
Integration test pass, looks good. Free to merge. |
2 participants
Description
sensor_dump()now checks the status of sensors before performing any read operations. If sensors ready, interrupts are disabled before any reads to prevent data race issues.Issue Link
Closes FCF #221
Testing
Testing Artifacts
Reviewer Checklist
Standards
Error Handling
Memory
Performance