ttlappalainen / NMEA0183

Library for handling NMEA0183 messages
71 stars 46 forks source link

Adding parsing for BOD, WPL, RTE, RMB, GLL message #5

Closed tonswieb closed 7 years ago

ttlappalainen commented 7 years ago

Hi,

You seem to have changed interface of library. I prefer to keep down compatibility. It can be done with simple inline functions on .h.

Are you using library to other direction? I am just modifying it to work as NMEA2000 library instead of changes what some other did and left things half way.

tonswieb commented 7 years ago

Hi,

Yes I did. Sorry for that. I could add the inline functions to the .h and leave the .cpp as is. Is that ok with you?

I am using it to connect my old Garmin GPS to B&G Triton display. I extended on your example to create a NMEA Gateway (https://github.com/tonswieb/NMEAGateway). The NMEA0183/2000 libs works great.

Regards, Ton

ttlappalainen commented 7 years ago

I think it is OK. I am not sure about the point to have own struct for each sentence. It looks cleaner for code, but anyway e.g. lat and long you can get from different messages.

Would you like to add MOB code to your device? I have also NMEA 0183 GPS -> NMEA 2000 device. On event it start to send waypoint to event position. On NMEA 2000 there is actually also MOB sentence, but it is unfortunately rarely supported.

tonswieb commented 7 years ago

I reverted the breaking API changes and added additional inline functions for the structs.

I used the structs for 2 reasons:

  1. By adding parsers for BOD, WPL, RTE, RMB, GLL we end up with a lot of standalone vars which I wanted to group in some way.
  2. I wanted to limit the number of shared vars, like in BoatData.h. Most of the vars, like GPS status information, can directly be used to sent NMEA2000 sentence. So there is no need to hold on to them. There are only a few exceptions like lat/lon, variation, date, etc...

The Garmin GPS outputs a MOB by making the active route a route with a single waypoint. I used that to change the route I send to B&G Triton. I haven't tried the MOB sentence itself. I am not sure if B&G Triton does anything with it, but I would like to try. Is the MOB sentence implemented in the NMEA2000 library?

ttlappalainen commented 7 years ago

I do not use MOB sentence, since it is rarely supported. It is not either currently on lib. Instead I start to send simple waypoint to the MOB pos. So it works as Garmin.

ttlappalainen commented 7 years ago

This does not compile: NMEA0183Messages.cpp:326:43: error: no matching function for call to 'push_back(const char*)' tRTE.wp.push_back(NMEA0183Msg.Field(i));

Also there is problem that you have definition std::list<char*> wp; and then you push pointer to it. If you then delete tNMEA0183Msg object, also pointers are not valid anymore! There is no automatic new char[xx]; strcpy(...) for push_back.

To keep code smaller, skip std:: totally and use some expected fixed size buffers. std:: is ok on computers, but I would avoid using of it for microcontroller. Specially this lib may be used for small project like uno, but now it is not possible.

tRTE could be class and behave as tNMEA0183Msg for wp names. So it would have one long char * buffer, where it copies wp names and list of pointers to wp names. Note also that you should check currSentence and follow that it is incremented by one for previous. If not, then wp list is damaged or restarted!

I made update, where I removed std:: I did not had time to test it and either had time to handle wp list. This must be thought more carefully. Keep in mind that library should work also on microcontrollers.

tonswieb commented 7 years ago

Ok. My bad. I had a look locally and realized that I forgot to fork and commit minor fix to https://github.com/maniacbug/StandardCplusplus. So I just did, https://github.com/tonswieb/StandardCplusplus. When you add this lib to Arduino IDE and compile with Arduino IDE you should not have any compilation issues. Although the compilation error you get is not the one I expected. So I assume that you are somehow compiling with another std:: lib.

That being said. I understand your concern regarding adding std:: on microcontrollers. I guess it is a trade off. It at least simplified handling routes and waypoints. For Arduino Mega this is not a problem. I can imagine for Uno it is.

The pointer problem with the std::list<char*> wp; will not occur when the tRTE is consumed directly and not reused for the next RTE message. I am using a single tRTE instance per RTE message and copy the wp to another list before continuing with next message. Have a look at https://github.com/tonswieb/NMEAGateway/blob/df1e6e86afd3d0b70214d654c93dfc096e997f37/NMEA0183Gateway.cpp#L350 This is also the place where currSentence is used. I also use std:: lib in this project for string, list and map and that works ok on Mega. It was my understanding that NMEA2000 lib does not run on Uno due to its size.

My idea was to keep the NMEA0183 lib stateless and leave the concat of RTE messages to the calling lib. In my case the NMEA Gateway.

I can concat wp's to one long char * for single RTE message. Then split again in NMEA Gateway.

ttlappalainen commented 7 years ago

Did you notice my commit without string? Just used some expected length fixed char buffers.

The way of pushing char pointers from msg may work in some cases, but is programatically wrong. Right way would be use std::list, but as I noted I am concerned even with Mega. For PC I like to use std:: libraries.

If the library does not concat RTE, then I understand why there is no checks. In that case RTE struct can just have buffer char[MAX_NMEA0183_MSG_LEN], since one message can not be longer than that anyway.

ttlappalainen commented 7 years ago

I updated library for different way (hopefully easier) to send messages. It should not have any changes for you RTE etc. code, so hopefully you can merger your project without problems.

Have you made any updates for parsing RTE?

tonswieb commented 6 years ago

I made an update for parsing RTE. Better late then never :-) I will open new PR for this.