Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring this into *the* rust nmea parser #9

Open
4 of 7 tasks
hargoniX opened this issue Mar 11, 2020 · 18 comments
Open
4 of 7 tasks

Refactoring this into *the* rust nmea parser #9

hargoniX opened this issue Mar 11, 2020 · 18 comments

Comments

@hargoniX
Copy link
Member

hargoniX commented Mar 11, 2020

It would be nice if rust finally had a go to nmea parser that is used by as many people as possible. Considering NMEA is a kinda easy data format to parse / handle usually this shouldn't be too hard to unify reasonably for everyone either. As of now there are 7 nmea related crates on crates.io.

  1. This one which with 4583 downloads seems to be the most popular one as of now
  2. https://github.com/nicolas-goudry/nmea-0183 by a member of this working group which has tests for each sentence as its most notable attribute.
  3. https://github.com/nsforth/nmea0183 this has no dependencies at all as well as no_std support as its most notable attributes.
  4. https://github.com/hargoniX/yanp (mine) which has the combination of support for lots of sentences + no_std support as its most notable attribute
  5. https://github.com/49nord/titanic-rs which can only parse GGA sentences, whether it has something special about it that might be interesting for us I don't know
  6. https://github.com/frafra/frakegps which emulates a GPS receiver that emits NMEA codes, however as it appears only for GGA code as well but maybe the ideas there could be useful for fuzzing related things.
  7. https://github.com/dndx/pitot which as far as I can see from isn't exactly useful for us? Feel free to correct me on this or any of the other statements though.

I believe that the core requirements for our NMEA parser should be that:

  • It should be no_std as things that use NMEA directly are often gonna be embedded devices. (see 3. and 4.) no_std #10
  • Should have as many tests as possible (see 2. and fuzzing 6.) Increase test coverage. #11
  • Should have as little requirements as possible, considering the size constraints on embedded devices (see 3.)
  • Should under no circumstances ever panic but always return an error, again in an embedded device there is nobody to look at your panic with their eyes. Don't panic #12
  • Should maintain API compatibility with the current crate so it is no hassle to upgrade for already existing users. @elpiel : We can break the API following semantic version and in the name of usability and idiomatic APIs
  • Shouldn't make use of alloc as for some embedded devices you either don't have or don't want to use an allocator for various reasons. Try to avoid alloc #13 no_std #10
  • Another cool and optional feature could be to expose our library functions and structs via the C FFI so they could be easily integrated as a piece of safe and fast Software into already existing C code bases or other code bases that can call into C. C API #14

It seems to me like especially the point with no dependencies seems kinda hard to adopt, especially when looking at timestamp data because it might be nice to have a well known data structure the user can use in his program. Parsing could be done without dependencies as 3. shows, however I don't think this should be our first target to tackle as it seems like a quite huge effort, would definitely be nice to have in a final version though.

@Dushistov
Copy link
Collaborator

@hargoniX

no dependencies seems kinda hard to adopt

I don't think that there is need for this. If dependecy can compile with no_std that is enough,
why remove such dependency?

@hargoniX
Copy link
Member Author

I'd assume the guy with the no dependency crate did it for size reasons of the resulting binaries but I agree, getting rid off a parsing library seems kinda overkill.

@Dushistov
Copy link
Collaborator

May be better to add all related users to cc?

@nicolas-goudry @nsforth @49nord @frafra @dndx

@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Mar 12, 2020

Thanks for this.

A few things:

Should have as little requirements as possible, considering the size constrains on embedded devices

Can you elaborate by "requirements"?

Should under no circumstances ever panic but always return an error, again in an embedded device there is nobody to look at your panic with their eyes.

This is a difficult thing to assert, so I expect this to be fine in the initial few versions. At least there shouldn't be an explicit panic.

@hargoniX
Copy link
Member Author

Requirements / Dependencies so our resulting binary doesn't grow overly large

Sounds good to me, of course we can take incremental steps towards these targets, question would just be what we are aiming at first.

@nsforth
Copy link

nsforth commented Mar 15, 2020

What a feature set and properties i expect from some "ideal" nmea parser as a developer of embedded device?

  1. It should support no_std enviroments. No exceptions.
  2. It should work without dynamic memory, but dynamic memory can be an option disabled by default.
  3. Must support RMC, VTG, GGA, GLL sentences because it's most needed and really useful. Many other is useful for debugging or GNSS quality control. Most other sentences is ancient crap and not emmited by receivers. (Read user manuals on modern receivers and you will not see them)
  4. Must be tested against many receivers. I saw some buggy receivers that violates NMEA specs, so ideal parser should have workarounds.

It will be nice to support additional vendor-specific protocol extensions. At least for widely used modern devices such as ublox.

C API is a strange thing. C developers typically try to avoid external dependencies written not on C, even C++ often is not welcome there.

P.S. My opinion based on many years of commercial embedded development.

@hargoniX
Copy link
Member Author

Two questions on this:

  1. Should I just not merge all the sentences I implement in yanp then and instead focus more on just rewriting it into no_std?

And for the C API, there is for example a team at my workplace that mainly does C/C++ dev on a very low level on x86 machines (writing drivers and such) and they were/are very interested in rust and some even commited a bunch of stuff to rustc to make it usable for their target so I think there are at least some people that might use this. Furthermore I do remember reading somewhere in the embedded-wg repos or websites or books that rust can be used as a more safe small part in bigger C code bases ideally so the devs don't have to completely rewrite their code base into rust but also not give up on Rust fully. But if you think it's a thing we don't need we don't have to do it of course.

@hargoniX
Copy link
Member Author

@nsforth

@IamfromSpace
Copy link

Hi all, I've been in need of a NMEA parser and was looking through my options, and figured I add something that hadn't been mentioned but is compelling to me personally: online parsing.

Because UART is going to stream it, I want a "parse as you read" interface, rather than passing in pre-chunked sentences. This is something I found useful from the nmea0183 crate, using it's iteration interface.

@nsforth
Copy link

nsforth commented Apr 1, 2020

@hargoniX I think no_std for nmea parser is a must. No matter of what crate is: yours or any other. I've rarely seen std-like enviroment on embedded devices for example some minimalized version of libc.
Dynamic memory allocation sometimes used but in limited implementations for example allocation of fixed sized blocks from pool. It is not usable for Box::new :)

@hargoniX
Copy link
Member Author

hargoniX commented Apr 1, 2020

@nsforth

That's not exactly what I asked, I wanted to know wether, based on your opinion that only a bunch of sentences is used anyways, do you think merging in the variety of sentences yanp supports is even worth it or should we just not support them and instead focus on other stuff.

@nsforth
Copy link

nsforth commented Apr 1, 2020

Okay, lets see what sentences yanp is supported. Especially for sentences usable for aerospace :)
BOD - I do not know where it is used now. May be on small boats?
BWC - Same as BOD
GBS - Intended to use by RAIM. May be useful for debugging, but i can't remeber when i have seen this.
GLC - Loran-C based GNSS backup, useful for big ships due to poor accuracy.
HDT - yet another marine useless sentence.
RMA-RMB - marine specific.

Do not focus on that crap. May be if someone really needs that or you have a bunch of additional time.

@nsforth
Copy link

nsforth commented Apr 1, 2020

@hargoniX

@hargoniX
Copy link
Member Author

hargoniX commented Apr 1, 2020

Alrighty, I'll look into no_std then!

@iamgabrielsoft
Copy link

Is this PR close?

@elpiel
Copy link
Member

elpiel commented Aug 19, 2022

@iamgabrielsoft This is not a PR but an issue. We've been working on a few of these points and completed them.
Mainly the no_std support (CC @nsforth).

For the rest of the people following the conversation, we're looking into:

  1. Extending the test cases Increase test coverage. #11
  2. Removing any panics (panic!(), except(), unwrap()) Don't panic #12
  3. Improving the API - this includes what @IamfromSpace mentioned: iteration interface - this I think is a perfect idea

I've also looked at NMEA 2000 to see if we can implement it but it seems that even getting the Appendix B Version (Database of Messages) is paid. I wonder what options we have apart from taking a look at other OSS libraries like https://github.com/ttlappalainen/NMEA2000 and replicating the parsing from it?
Although I haven't taken a deep dive into the library (and haven't done much C++).

CC @ttlappalainen as they might also be interested in this discussion.

@ttlappalainen
Copy link

I got this as cc and quickly read through so I do not have complete idea about your library and use of it. As NMEA 2000 "specialist" I give some comments about it.

  • First remember that if you buy NMEA 2000 Appendix B Version, your need to write NDA and then forget to publish anything on open source.
  • My NMEA 2000 library is fully featured and certification ready. There are several commercial certified producst based on it.
  • Library uses c++ and new to allow easier dynamic memory size allocation. With small MCU:s for simpler tasks one can define smaller buffers. it could have been done with static allocation and defines, but as c++ programmer I did not like the idea. Buffers will be allocated once, so it does not make big difference to static allocation.
  • It does not yet use std.
  • Remember that there is no standard protocol for NMEA 2000 data except on CAN. Outside CAN there are different "protocols" to show NMEA 2000 messages. As a sample Actisense, Maretron, Seasmart. This seem to be most confusing for people.

Feel free to ask, if you have more questions about NMEA 2000. I have worked with it about 7 years and last few years as profession so I think I know something about it.

@elpiel
Copy link
Member

elpiel commented Aug 21, 2022

@ttlappalainen thanks for the response, I really appreciate it!
Since there currently isn't a Rust crate (library)* for NMEA 2000 I thought it would be very beneficial to extend this crate and support it too. I personally don't have experience working with either 0183 or 2000, so it's a new area for me to explore.

If we decide to work on the 2000 implementation, we might look into other solutions like bindings for your C++ library, especially since you mentioned that it's "fully featured and certification ready". I'll make sure to reach out to you again when we start working on the 2000 implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

8 participants