AeroRust / nmea

NMEA 0183 - for communication between marine electronics such as echo sounder, sonars, anemometer, gyrocompass, autopilot, GNSS receivers and many other types of instruments. Defined and controlled by the National Marine Electronics Association (NMEA)
https://crates.io/crates/nmea
Other
61 stars 41 forks source link

Enable sentences with features #91

Closed elpiel closed 1 year ago

elpiel commented 1 year ago

We should have all sentences enabled with a feature, e.g. "all-sentences", however, it should be possible to parse only a subset of sentences by enabling only their own feature, e.g. gll or gsv, etc.

This will ensure that the size of the crate is optimised for highly constraint targets which require some subset of the sentences.

This is related to #54

PegasisForever commented 1 year ago

I guess this will be a breaking change? If we make all-sentences the default feature, applications use default-features = false for no-std support will break

elpiel commented 1 year ago

Valid point. Yes it will be, but we'll bump version to 0.5. We can also include Changelog but I'm not afraid to break the build from minor version updates.

default = ["std", "all-sentences"]
all-sentences = ["...", "vendor-specific"]
vendor-specific = [".."]
...
..
.
Dushistov commented 1 year ago

What about usage of macros instead of features?

Something like

generate_parser![SentenceType::AAM, SentenceType::ABK];

and that generate nmea parser that parse only AAM and ABK messages, so compiler+linker in release mode can remove unused part of nmea crate?

Plus it would be nice implement the similar fnctionality for Nmea::create_for_navigation.

This variant of dealing with redundant code allows the compiler to solve the unnecessary code problem, instead of manually marking of each sentence with feature.

elpiel commented 1 year ago

@Dushistov macros might make it more difficult to use the crate and contribute to the project IMO. Also, not all tools/ides/editors play nicely with macros. On no_std crates, I've seen macros used to implement different peripherals but not exposing the internal macros for users to define their own.

It's tedious process to add these features now but explicitly listing them sounds like a very good approach. Due to the way parsers work, I suppose we can even add "Feature not enabled" error when a sentences is passed which can be parsed but the feature hasn't been enabled.

elpiel commented 1 year ago

There's one more additional benefit that I though of. We can leave the parsers themselves and just exclude the calling of the parser in the parse_str function.

Compiler will still optimize and remove the unused parser/code, however, if you want to parse a specific sentences in your project which you have excluded from parse_str, then it's your choice and you can do it without enabling the feature.

ekuinox commented 1 year ago

In my case, I use my own ParseResult-like enumeration type combined with some public parsers instead of using ParseResult. So I think it is a good change to be able to exclude unused sentences from the compilation by means of a feature flag.

elpiel commented 1 year ago

These sentences features have been implemented in #94 . 0.5 release introduce this breaking change, however, we do not have guarantees for keeping the API for minor version bump and we do have other breaking changes as well so this is ok.

Closing this issue for now as it's Done :heavy_check_mark: . If anyone has suggestions on how to improve those don't hesitate to open issues or PR!