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

Nmea listener, parser and module to master #7

Open
wants to merge 87 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@itzandroidtab
Copy link

commented Jun 4, 2019

@streefje1996

This comment has been minimized.

Copy link

commented on 831124b May 17, 2019

for class diagram reference see: https://drive.google.com/open?id=1mMdPIAQqadgT6fmVWsLVA1Dnb0JzsxD6

  • parse.cpp and main.cpp contain the exact same code;
  • parse() is a function from abstract class: location_detection_c;
  • struct location_s needs to be updated according to class diagram;
  • time_maker function looks good maybe a different name would be better but it looks good, maybe write some tests to see if it outputs what you think it should;
  • latitude_longitude_maker needs to be updated to work with floats instead
  • in the parser function, maybe try editing the sizes of your temp values like hwlib::string<10> north_or_south_temp; could be hwlib::string<1> north_or_south_temp; since it only contains one char, maybe even change it to a single char;
  • comma delimiter looks alright, maybe write some tests;

in general:

  • don't forget to add the r2d2 namespace
  • don't forget to implement the function from the abstract class
  • don't place an int main in source code from a class

Keep up the good work :)

Official12345 and others added some commits May 20, 2019

Added description for the class itself.
Added description for the class itself.
Working on the implementation of the location detection module
Specifically the get_location() and compress() functions are being worked on. As well as doxygen documentation for multiple headers.
Working on the process() function of location_detector
It isn't finished yet, because we do not know what frame we are listening for.
Merge pull request #4 from R2D2-2019/feature-uart_nmea_class
Merging functions with nmea class

itzandroidtab and others added some commits Jun 4, 2019

* https://www.gpsinformation.org/dale/nmea.htm#GGA
*
*/
struct gga_s {

This comment has been minimized.

Copy link
@LRstudentHU

LRstudentHU Jun 4, 2019

It might be worth reordering members for better packing.

This comment has been minimized.

Copy link
@itzandroidtab

itzandroidtab Jun 4, 2019

Author

I think the current ordering is okay. As this is the order the data is arranged in the NMEA-GGA message. This does use more memory but as we only have a single location at the time it shouldn't matter that much

Show resolved Hide resolved code/headers/nmea_listener.hpp Outdated
Show resolved Hide resolved code/headers/nmea_listener.hpp Outdated
Show resolved Hide resolved code/headers/nmea_listener.hpp Outdated
Show resolved Hide resolved code/headers/nmea_listener.hpp Outdated
Show resolved Hide resolved code/headers/string.hpp Outdated
Show resolved Hide resolved code/src/string.cpp Outdated

frame.altitude = altitude;

frame.long_deg = static_cast<uint8_t>(longitude / 100);

This comment has been minimized.

Copy link
@LRstudentHU

LRstudentHU Jun 4, 2019

The only thing these coordinates are used for is for sending on the bus and they get converted to an integer here. Is there any point in storing them as floats in the first place? Removing all these floating point operators would vastly improve performance.

This comment has been minimized.

Copy link
@itzandroidtab

itzandroidtab Jun 4, 2019

Author

In my point of view there was no need to store anything in floats. The previous team made this decision. To remove this we might need to ask permission to the PO first as we need to change more things than the conversion method. So yes we can remove the usage of floats.

itzandroidtab and others added some commits Jun 4, 2019

Merge pull request #9 from R2D2-2019/hotfix-default_gga_values
added default values to parse_nmea
Merge pull request #10 from R2D2-2019/feature-parse_nmea_function
Updated the travis.yml to the new version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.