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

Example - full log of sentences #39

Merged
merged 2 commits into from
Apr 17, 2022
Merged

Example - full log of sentences #39

merged 2 commits into from
Apr 17, 2022

Conversation

elpiel
Copy link
Member

@elpiel elpiel commented Apr 17, 2022

Not all sentences get parsed, however, there are a few sentences that can be used for testing related to #11 :

E.g.

Galileo:

GSV(
            GsvData {
                gnss_type: Galileo,
                number_of_sentences: 2,
                sentence_num: 1,
                _sats_in_view: 8,
                sats_info: [
                    Some(
                        [Galileo,2,Some(56.0),Some(46.0),None],
                    ),
                    Some(
                        [Galileo,3,Some(11.0),Some(149.0),None],
                    ),
                    Some(
                        [Galileo,7,Some(54.0),Some(298.0),None],
                    ),
                    Some(
                        [Galileo,8,Some(60.0),Some(174.0),None],
                    ),
                ],
            },
        ),

GPS

        GSV(
            GsvData {
                gnss_type: Gps,
                number_of_sentences: 3,
                sentence_num: 3,
                _sats_in_view: 12,
                sats_info: [
                    Some(
                        [Gps,28,Some(0.0),Some(0.0),Some(17.0)],
                    ),
                    Some(
                        [Gps,30,Some(28.0),Some(84.0),Some(19.0)],
                    ),
                    Some(
                        [Gps,19,Some(23.0),Some(147.0),None],
                    ),
                    Some(
                        [Gps,20,Some(3.0),Some(201.0),None],
                    ),
                ],
            },
        ),

@codecov
Copy link

codecov bot commented Apr 17, 2022

Codecov Report

Merging #39 (b0d4f2a) into master (c985682) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #39   +/-   ##
=======================================
  Coverage   84.52%   84.52%           
=======================================
  Files          12       12           
  Lines         982      982           
=======================================
  Hits          830      830           
  Misses        152      152           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c985682...b0d4f2a. Read the comment docs.

@Dushistov
Copy link
Collaborator

Why example and not unit or functional test?

@Dushistov
Copy link
Collaborator

And forgot to say, thanks for nmea log and contribution.

@elpiel
Copy link
Member Author

elpiel commented Apr 17, 2022

Why example and not unit or functional test?

Since I don't really test the results but only the length of the resulted parsed sentences, I thought that it makes more sense as an example.
I can change it to tests as well, I don't mind.

PS: I'm more interested in the no_std support and that's where I need help to see how we can remove all allocations

@Dushistov
Copy link
Collaborator

Ok, my main concern is new dependencies. log is not big crate, but it is pitty to add new crate that is not used by crate.
Test can solve that issue, because of cargo test suppress output by default, so there is no need for these extra crates: log and env_logger.

But I suppose I can merge as is and fix it latter.

@Dushistov Dushistov merged commit 9997ad9 into master Apr 17, 2022
@Dushistov Dushistov deleted the full_sentences_example branch April 17, 2022 18:45
@elpiel
Copy link
Member Author

elpiel commented Apr 17, 2022

@Dushistov It would be nice to take advantage of log throughout the crate for debugging, especially the parsing part. This is the main reason I've added it and about env_logger - this is the easiest way to add logging for examples or tests or whatever. Maybe even a CLI tool for parsing such sentences would be nice as a tool and env_logger could again be used for debugging.

What do you think?

@Dushistov
Copy link
Collaborator

@elpiel

It would be nice to take advantage of log throughout the crate for debugging, especially the parsing part

Not sure about this, logging is not free, in particular if we talk about no_std.

info!(hi) is transformed into:

    {
        let lvl = ::log::Level::Info;
        if lvl <= ::log::STATIC_MAX_LEVEL && lvl <= ::log::max_level() {
            ::log::__private_api_log(
                ::core::fmt::Arguments::new_v1(&["hi"], &[]),
                lvl,
                &("foo", "foo", "src/main.rs", 13u32),
                ::log::__private_api::Option::None,
            );
        }
    };

This is bunch of extra instructions that pollute instruction cache and use main memory even if logging is disabled, plus bunch of constant strings.
I am not sure that no_std developers would be fond of extra kilobytes of memory just for string literals,
especially if they are not use logging.

And we not deal with something that may fail even if our code is fine, like
files, network and so on. Why use extra memory for logging if we just take string parse it and return result?
User can just log the errors if he/she wants logging.

Maybe even a CLI tool for parsing such sentences would be nice as a tool and env_logger could again be used for debugging.

Yes, but cargo still have no extra section for bin-dependicies, so it should be in separate crate if it uses some extra dependencies. But may be in the same repository via workspace cargo feature.

@elpiel
Copy link
Member Author

elpiel commented Apr 17, 2022

It does make a lot of sense to not include log crate, however, I think we can remove it once we make the crate no_std and see if this really makes a difference.

I just think it would help a lot with the debugging and in general the development process.

As for bin-dependences, I know this, but yet again it's only going to pollute the dev. dependencies and if it helps the dev. process it's fine by me.

My bigger concerns are to make the crate no_std and test it as we can always improve performance and dependencies later on.

@Dushistov
Copy link
Collaborator

I suppose this is not important now. We discuss something that it not really used right now.
I mean log is used by examples not crate by it self.
So when it was really used we can return to this discussion.

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

Successfully merging this pull request may close these issues.

2 participants