search

Openvario / variod

Daemon for autonomous e-vario
4 stars 8 forks source link

addressing issues listed in #20 #30

Closed bomilkar closed 4 years ago

bomilkar commented 4 years ago

It improves robustness of NMEA parsers. It presents one NMEA sentence at a time to be parsed. It follows the guidelines for NMEA sentence structure given here: https://en.wikipedia.org/wiki/NMEA_0183#Message_structure with listed exeptions: NMEA sentences are only valid if they have a checksum. The at the end of the sentence is not enforced. One of the 2 is sufficient, but not less than 1 is required.

The next steps: Implement the request to XCSoar to send current settings. Handle incomplete senteces from recv(). Both are marked in the code as TODO:

bomilkar commented 4 years ago

I admit: it's a major overhaul of the code. But nothing less is required in my opinion. Smaller chunks cannot be identified. There is no point crossing the river half way.

I think I do address the remaining Codacy issues at runtime. Let me know if you disagree. They are of the form "Does not handle strings that are not \0-terminated". Those strings are always 0 terminated when it gets there (I think).

linuxianer99 commented 4 years ago

@bomilkar : I think it make more sense to add the software tests to variod instead of new features. Also this PR and the commits can be easily split up into logical functions to have smaller and better reviewable commits:

Please do not mix up functions in commits/PRs ... We discussed this already. There should also be a unit test for every function. I am looking at cmocka (https://cmocka.org/) at the moment. Looks very promissing !!

bomilkar commented 4 years ago

Checksum needs changed parser to work and vice versa. There are no independent commits. I've exhausted them with the previous 2 PRs. Close the PR if you don't like it the way it is and we stop wasting everybody's time. Take it or leave it.

bomilkar commented 4 years ago

I fixed memory leaks, eliminated empty code, fixed potential segmentation faults, removed ambiguities, etc. These are no "new features", at least not in my book. I was just cleaning the crap. If you ask @kedder for a review I'm sure he will agree, There is no point running unit tests as long as the code is full of obvious bugs.

BTW: I'm disappointed with Codacy: it complains on functions like strlen() as a potential risk, but it fails to find use of uninitialized variables (like the infamous sock_err issue we had a few days ago). It seems to be good at limiting the scope of variables which I consider to be of limited value.

linuxianer99 commented 4 years ago

@bomilkar : I am not saying you did not fix anything !! The only thing i am asking for is, splitting up the things in smaller pieces to be able to test the stuff. So why not one commit for every logical change ?

You added complete new files (nmea_checksum) fixed various leaks. That's all fine, but nobody can see the dependency of the changes.

I am fine if it is one PR, but also this can be splitted up to keep track of all the changes. Let's bring in the stuff step by step and not in one commit please !

Nevertheless i think we should make a unit test mandatory for every new C-function introduced in the code ...

Codacy: There are some other tools like this. Maybe we can improve this in the future.

kedder commented 4 years ago

I'm with @linuxianer99 on this - if you reorganize the change into several commits, each of them dealing with one particular bug or problem, it will be much easier to:

"One commit with bunch of unrelated stuff in it + fixup commits on top" style just makes all of the above a big PITA (long after original author is not available to support their code anymore). The reason all maintainers are asking for this is not because they are arrogant nitpicks, but because otherwise it is going to be their problem eventually.

Git has all the tools for that, it is just a matter of learning how to use them. There are tons of articles online explaining these tools in simple terms.

Having tests of course make sense on any stage of the development. Especially when you fix the bug and don't want it to get reintroduced. You reproduce the problem in a test and fix it to make test pass. Of course, currently variod doesn't have any infrastructure for automated tests, and it takes additional effort to introduce it. And I'd say this infrastructure is out-of-scope of this particular PR.

For the codacity, I think it is a valuable tool, just needs to be configured by blacklisting checks we don't care about. Ideally all PRs should get green, either by addressing the issues in the code or by configuring Codacity when these issues do not make sense. I'm pretty sure there should be some kind of configuration for it somewhere.

bomilkar commented 4 years ago

So you are suggesting to remove the memory leaks (removing malloc()) in one commit and replace these exact same lines in the next step?? You think this makes it easier to understand? The parser can't verify the checksum if it doesn't know where in the sentence it's at. It frequently uses a pointer form strtok() w/o checking if it's null. Do you want me to fix this first and then in the next commit replace that fix with a better solution? You want more examples??? Who wrote/reviewed the code in the first place? Maybe both of you live under the assumption the code was in good shape and can be fixed or improved. But the fact it was in there for years doesn't imply anything. Take 5 minutes and read through nmea_parser.c (just one example). You'll see it's up for a re-write. No no. This makes no sense at all. Take it or leave it. I'm done with it.