Added checks to discard gps data if RMC or GLL status is invalid - #32
Added checks to discard gps data if RMC or GLL status is invalid#32Bigmouseears wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
LGTM!
Technically should be ready to go, but I'd like some robustness cases put in place to exercise the new invalid paths. We can sync on what updating the tests should look like @Bigmouseears.
If you want to get started on them ahead of time, look in driver/_test/driver/gps/test_gps.c:test_GPS_parse, cases/NMEA_inputs.txt, and cases/gps_parse_expected.txt
|
I have it so that GPS data values are all set to 0 if data is invalid. Not sure if that will break anything so I guess it's worth mentioning. |
|
Wow i'm surprised it passed the unit tests. I wonder if I may have done the tests wrong? |
|
Test request: Add a case where there's data in the GPS struct already, and you receive an invalid message, confirm that the preexisting data is not overwritten |
|
OH dude that explains so much, the driver CI is borked for unit tests and doesn't fail the job on a test fail. Messages 9 and 15 fail to parse on the runner. Statement coverage is achieved. |
…ata. Also added two new test cases.
ETSells
left a comment
ETSells
left a comment
There was a problem hiding this comment.
Code updates look good, test cases show the result I expect, and CI runs of the test shows coverage passes the required spec and the results pass! Ship it!
| /*------------------------------------------------------------------------------ | ||
| Global Variables | ||
| ------------------------------------------------------------------------------*/ | ||
| GPS_DATA gps_last_valid_rmc_data; |
There was a problem hiding this comment.
hmm... works but id like us to consider encapsulating these to GPS's scope
There was a problem hiding this comment.
ah, forgive my incompetence. For some reason I thought that "global" meant just visible across all functions in the file, and nowhere else.
I guess I can just make them static variables within the function then, like this (if the image works)
There was a problem hiding this comment.
gps tests still pass with this implementation, at least on my end and if I didn't screw anything else up
| } | ||
| else if (gll_status == 'V') | ||
| { | ||
| *gps_ptr = gps_last_valid_gll_data; |
There was a problem hiding this comment.
nit: indentation should follow standards
| gps_ptr->gll_status = gll_status; | ||
|
|
||
| /* Save rest of data only if status is A: Data Valid */ | ||
| if (gll_status == 'A') |
There was a problem hiding this comment.
oof ik i didnt follow parentheses spacing standards when i wrote this function so this is fine for consistency. still sadge, don't ever let sophomore me write code again
2 participants
Description
A brief description of the changes in the PR
Issue Link
Please provide a link to the issue (e.g. "Closes #1").
Also, if this PR is one of multiple for this issue, link the parent if this is a child OR link
the children if this is the parent. Do not use "closes" keywords on child PRs, only use them on
the parent.
Testing
Attach any test artifacts here, if relevant.
Other
Leave any additional notes here
Reviewer Checklist
Standards
Error Handling
Memory
Performance