ttlappalainen / NMEA2000

NMEA2000 library for Arduino
531 stars 220 forks source link

Attempt at Parse129285 #369

Open dagnall53 opened 9 months ago

dagnall53 commented 9 months ago

Timo. I have been trying to write a simple Parse129285 to extract (just) the origin and destination waypoint numbers and names. But the code crashes with a "_0x401036cf: __stack_chk_fail at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/esp_system/stackcheck.c line 36" that is seemingly triggered by the WPID= line in the code below.

It seems to me that probably the Index is longer than the message (hence the abort?) , and perhaps there is "more" message somewhere, but I am not proficient enough to find where I have gone wrong.. When its working I would be pleased if you wanted to add it to the library! I have loads of Serial prints in it at the moment to try and help me understand what is going wrong and where!

I have added a text file with the code.

`

bool ParseN2kPGN129285(const tN2kMsg &N2kMsg, uint16_t &WPID,  char *OriginWP, uint16_t &WPID2,char *DestWP){
      if(N2kMsg.PGN != 129285L) return false;
        int Index;
        uint16_t NumItems,temp,StartRPS;
        double Latitude,Longitude;
        char RouteName [NMEA0183_MAX_WP_NAME_LENGTH];
        unsigned char vb;
        byte TempB;        
    Index = 0;
    StartRPS=N2kMsg.Get2ByteUInt(Index);     Serial.printf("\r\n index[%i] StartRPS:%i",Index,StartRPS);
    NumItems = N2kMsg.Get2ByteUInt(Index);   Serial.printf("\r\n index[%i] Num Items:%i",Index,NumItems);
    temp=N2kMsg.Get2ByteUInt(Index);         Serial.printf("\r\n index[%i] datbaseID:%i",Index,temp);//databaseID
    temp=N2kMsg.Get2ByteUInt(Index);         Serial.printf("\r\n index[%i] RouteID:%i",Index,temp);//Route ID
    vb=N2kMsg.GetByte(Index);                Serial.printf("\r\n index[%i] supp data :%0Xh ",Index,vb);//supp data and NAV direction and reserved bits
    N2kMsg.GetStr(RouteName,20,Index);       Serial.printf("\r\n index[%i] route:",Index);Serial.print(RouteName);
    vb=N2kMsg.GetByte(Index);                Serial.printf("\r\n index[%i] reserved byte :%0Xh ",Index,vb);
           // and here it crashes!  ?? why ??
    WPID= N2kMsg.GetByte(Index);             Serial.printf("\r\n index [%i]  WP1   %i",Index,WPID);                        // this is the line that crashes with a stack check crash.. 
    //N2kMsg.GetStr(OriginWP,20,Index);
    // Latitude = N2kMsg.Get4ByteDouble(1e-07, Index);
    // Longitude = N2kMsg.Get4ByteDouble(1e-07, Index);
                                // next item
    // ID2= N2kMsg.Get2ByteUInt(Index);                        
    // N2kMsg.GetStr(DestWP,NMEA0183_MAX_WP_NAME_LENGTH,Index);
    // Latitude = N2kMsg.Get4ByteDouble(1e-07, Index);
    // Longitude = N2kMsg.Get4ByteDouble(1e-07, Index);

    //Serial.printf("\r\n  Parse 129285 origin[%s] , dest[%s]\r\n",OriginWP,DestWP);
   return true;
   }

`

Monitor prints out:

index[2] StartRPS:1 index[4] Num Items:2 index[6] datbaseID:2 index[8] RouteID:3 index[9] supp data :E1h index[29] route:N/A⸮ index[30] reserved byte :4Dh index [31] WP1 23 Stack smashing protect failure!

ParseN2kPGN129285.txt

ttlappalainen commented 9 months ago

There is several errors in your code.

  1. You can not parse that PGN with single function.
    • There should be one function to get brief information of data withing message - Route information and number of WP in message.
    • Then you other function to request nth WP data from message. For WP data it is best to create structure including provided WP fields.
  2. To make parser you need exact information of message content. Route name and WP names are variable strings and you are trying to read them with fixed string function.
  3. You should not mix NMEA0183 code to NMEA2000 code (NMEA0183_MAX_WP_NAME_LENGTH)
  4. You are providing just char *DestWP, but not size of buffer, which is rather bad and unsafe programming. Your code crashes since you are requesting fixed length string of size NMEA0183_MAX_WP_NAME_LENGTH, but your DestWP either does not point buffer big enough or to existing buffer at all.
dagnall53 commented 9 months ago

Ah, I was arriving slowly at the conclusion that the variable length strings were the problem, So I presume that the inital message contains data about how long the variable strings actually are?-

So if I understand correctly: SOMEWHERE (?) there is data in the first part of the message that tells me the length of the variable strings in the rest of the message. (Are you able to help with this or is this part of the hidden and proprietary NMEA2000 details??).
I can see that with this data about the length of the strings I can then repeat a nice structured read of N subsequent WP ID Name, Lat Long.. I did look for other examples of code to try and learn string extraction with variable lengths.
Can you direct me to a (good) example parse that uses variable length strings so that I can learn some more!!

Lastly: Would you reccomend passing the size of the char* buffers to the function? My simplistic assumption was that providing they were big enough, they would ‘just’ be filled with (however many) bytes from message.
I was avoiding a proper WP structure for now, while I tried to get some data. I can improve that. (And sorry about mixing in the NMEA0183_MAX…. I can avoid that!)

Many thanks Dagnall

ttlappalainen commented 9 months ago

Just use size_t DataLen=RouteNameBufLen; N2kMsg.GetVarStr(DataLen,RouteName,Index);

Function return true/false according success. On DataLen string lengt will be returned, if there is any interest. Read the manual: https://ttlappalainen.github.io/NMEA2000/classt_n2k_msg.html#a2fa9d6983da6de6729568ab7f334d7ac

dagnall53 commented 9 months ago

Many thanks! Like many - I must apologise for not having read the manual!! I now have it bookmarked! I had derived my own version of the variable string read, but had not appreciated the meaning of the second byte, which I had just skipped.

    len=N2kMsg.GetByte(Index);        
    Index++;
    if(len>>18){ return false;} 
    N2kMsg.GetStr(Waypoint.Name,len-2,Index);   `

I took your advice and started to learn OOC.. and now have a working set of 129285 functions, I append the EXT part below.. Much tidier now I have the GetVarStr. Many thanks!


struct tWAYPOINT {
  uint16_t ID;
    char Name[20];
    double latitude;
    double longitude;
    };

tWAYPOINT Waypoint[5];

bool ParseN2kPGN129285ext(const tN2kMsg &N2kMsg, int &Index, tWAYPOINT &Waypoint){  
  /*10 Set 1 WP ID      0 .. 65533 16 bits unsigned NUMBER
    11 Set 1 WP Name            Variable length STRING_LAU
    12 Set 1 WP Latitude        1e-07 deg -214.7483647 .. 214.7483645 32 bits signed NUMBER
    13 Set 1 WP Longitude       1e-07 deg -214.7483647 .. 214.7483645 32 bits signed NUMBER
  */
    size_t STRING_LAU=20; //My buffer length
    if (!(Waypoint.ID = N2kMsg.Get2ByteUInt(Index))) return false;   
    if (!(N2kMsg.GetVarStr(STRING_LAU,Waypoint.Name,Index))) return false;
    if (!(Waypoint.latitude = N2kMsg.Get4ByteDouble(1e-07, Index))) return false;
    if (!(Waypoint.longitude = N2kMsg.Get4ByteDouble(1e-07, Index))) return false;
    return true;
}
``` `
ttlappalainen commented 9 months ago

Use:

struct tN2kWaypoint { double latitude; double longitude; uint16_t ID; char Name[62]; };

With this order you do not loose memory due to padding. Name can be max 30 chars, but may be unicode (currently not supported on library) and so take 60 bytes.

Then function name could be more informative like ParseN2kPGN129285NextWaypoint and inline alias like ParseN2kRouteNextWaypoint

Instead of using size_t STRING_LAU=20; use size_t NameLen=sizeof(Waypoint.Name); then it gets size automatically from tWaypoint::Name definition.

I noticed that documentation does not tell output of StrBufSize. It outputs size of string in message. So GetVarStr returns true even if buffer is too small, but outputs size it would have needed.