Ensure that AIS fixed strings are ITU-R M.1371-1 compliant

[lsoltero]

Check and modify content of fixed strings passed to AIS library to ensure the are ITU-R M.1371-1.

[lsoltero]

Hi Temo,

Is there an action item for me here? It looks like the build is failing but as far as I can tell this has to do with a git permission error.

/usr/bin/git push origin gh-pages
remote: Permission to ttlappalainen/NMEA2000.git denied to github-actions[bot].
fatal: unable to access 'https://github.com/ttlappalainen/NMEA2000.git/': The requested URL returned error: 403
Error: Action failed with "The process '/usr/bin/git' failed with exit code 128"

[ttlappalainen]

It is Timo!

Remember also change version in library.properties!

Setter AIS string comments like on line 4290 should be "Call Sign - Max. 7 chars will be used. Input string will be converted to contain only SixBit ASCII character set (see. ITU-R M.1371-1)"

I succested to create function for conversion under N2kMessages module not to tN2kMsg class. tN2kMsg contain mainly functions for N2k data formats and AIS message is not specific DF in N2k. On the other hand naturally it would be more efficient to do conversion on tN2kMsg, but on the other hand it increases size of class for users who does not need AIS. Linker unfortunately does not leave away unused class functions. Current solution increases the size and also makes double copy by using temporary buffer for conversion.

Hopefully few bytes more does not matter, so AddAISStr could be.

void tN2kMsg::AddAISStr(const char *str, int len) {
unsigned char *buf=Data+DataLen;
for (; len>0 && *str!=0 && DataLen<MaxDataLen; len--, DataLen++, buf++, str++) {
char c = toupper((int)*str);
*buf=(c >= 0x20 && c <= 0x5F) ? c : '?';
}

len=min(len,MaxDataLen-DataLen);
if ( len>0 ) memset(buf,'@',len);
}

[lsoltero]

I like the idea of having AddAISStr in the tN2kMsg class since it makes the code in N2kMessages cleaner. I will make the requested changes and resubmit the PR.

[lsoltero]

hello timo,

updated PR with the requested changes. Updated library.properties and library.json. still not understanding why getting build errors in pull request...

Push the commit or tag
/usr/bin/git push origin gh-pages
remote: Permission to ttlappalainen/NMEA2000.git denied to github-actions[bot].
fatal: unable to access 'https://github.com/ttlappalainen/NMEA2000.git/': The requested URL returned error: 403
Error: Action failed with "The process '/usr/bin/git' failed with exit code 128"

[ttlappalainen]

Better to remove #include and just use simple
if ( len>MaxDataLen-DataLen ) len=MaxDataLen-DataLen;
AVR may not have . I have not tested but there are other std libraries not available on AVR.

Did you test changed code?
Did you test it with too long strings? Feed e.g., "TooLongCallSign" to call sign.
Also should be tested with short strings like "CS@S" should fill "CS@S@@@". I do not know how they show up.
And with invalid characters. "CS\tS"

[lsoltero]

Hi Timo,

the version that has been tested and is now in production is the previous pull where

//*****************************************************************************
// Add AIS String
// make sure characters fall into range defined in table 14
// https://www.itu.int/dms_pubrec/itu-r/rec/m/R-REC-M.1371-1-200108-S!!PDF-E.pd
void tN2kMsg::AddAISStr(const char *str, int len) {
char AISStr[21]; // Max AIS strlen defined in ITU-R M.1371-1
size_t i;
for (i = 0; str[i] != '\0' && i < sizeof(AISStr); i++ ) {
char c = toupper((int)str[i]);
AISStr[i] = (c >= 0x20 && c <= 0x5F) ? c : '?';
}
AISStr[i]='\0';
AddStr(AISStr,len,false,'@');
}

This has been tested for fill and illegal characters and it works.

What has not been tested are your suggested changes that address concerns for code space in small embedded systems. Personally I like my implementation of AddAISStr where a separate buffer is used to build the filtered string before passing it to AddStr(). I like it because

1. its more readable
2. does not depend on the knowledge of the internal structures maintained by N2kMsg
3. does not replicated code
4. uses AddStr()/SetBufStr() which is established and well tested code that has been in the library forever.

On the surface your suggested changes look good and have low risk of breaking anything... But... we are not inclined to test it since it's not our intention to use it.

so my suggestion is to revert back to the double buffering version or write code to spot check your version or maybe get someone with an Arduino to test it.

Let me know what you would like me to do on my end.

--Luis

[ttlappalainen]

What do you mean with "we are not inclined to test it since it's not our intention to use it."

[lsoltero]

Hi Timo,

we keep our own fork of the project with many modifications. So we have not incorporated these changes

void tN2kMsg::AddAISStr(const char *str, int len) {
unsigned char *buf=Data+DataLen;
for (; len>0 && *str!=0 && DataLen<MaxDataLen; len--, DataLen++, buf++, str++) {
char c = toupper((int)*str);
*buf=(c >= 0x20 && c <= 0x5F) ? c : '?';
}

len=min(len,MaxDataLen-DataLen);
if ( len>0 ) memset(buf,'@',len);
}

and don't plan to given that we like the alternate implementation better. And we have it in production and it's a big deal and inconvenience to get new code to our testers in the field and receive feedback.

so this small change is not worth pursuing for us.

summing up...

this has been tested...

//*****************************************************************************
// Add AIS String
// make sure characters fall into range defined in table 14
// https://www.itu.int/dms_pubrec/itu-r/rec/m/R-REC-M.1371-1-200108-S!!PDF-E.pd
void tN2kMsg::AddAISStr(const char *str, int len) {
char AISStr[21]; // Max AIS strlen defined in ITU-R M.1371-1
size_t i;
for (i = 0; str[i] != '\0' && i < sizeof(AISStr); i++ ) {
char c = toupper((int)str[i]);
AISStr[i] = (c >= 0x20 && c <= 0x5F) ? c : '?';
}
AISStr[i]='\0';
AddStr(AISStr,len,false,'@');
}

but this

void tN2kMsg::AddAISStr(const char *str, int len) {
unsigned char *buf=Data+DataLen;
for (; len>0 && *str!=0 && DataLen<MaxDataLen; len--, DataLen++, buf++, str++) {
char c = toupper((int)*str);
*buf=(c >= 0x20 && c <= 0x5F) ? c : '?';
}

len=min(len,MaxDataLen-DataLen);
if ( len>0 ) memset(buf,'@',len);
}

has not and we don't intend to test.

Let me know if I can be of further assistance.

--Luis

--luis

[ttlappalainen]

In your tested code:

• You always do conversion for full string even final length would be 7
• If string of length >=21 chars will be provided, you will have trouble. You will stop for loop to test i < sizeof(AISStr) meaning i==21. Then later you do AISStr[i]='\0';

Have to confess that I noticed some time ago that either AddStr has not been tested completely and it may write over Data array. That was one reason for rewriting AddAISStr.

[lsoltero]

Hi Timo,

well this is where len comes in.. in my code I make sure that Len is set correctly for 794,809, and 810

void SetN2kPGN129794(tN2kMsg &N2kMsg, uint8_t MessageID, tN2kAISRepeat Repeat, uint32_t UserID,
uint32_t IMOnumber, const char *Callsign, const char *Name, uint8_t VesselType, double Length,
double Beam, double PosRefStbd, double PosRefBow, uint16_t ETAdate, double ETAtime,
double Draught, const char *Destination, tN2kAISVersion AISversion, tN2kGNSStype GNSStype,
tN2kAISDTE DTE, tN2kAISTranceiverInfo AISinfo)
{
...
N2kMsg.AddAISStr(Callsign, 7);
N2kMsg.AddAISStr(Name, 20);
...
N2kMsg.AddAISStr(Destination, 20);
}

so the above works... as long as the User passes in strings that are not longer than 21 characters... as we do in our code. But yes.. you are correct the loop terminator should be i < sizeof(AISStr) -1; to be safe... thanks for pointing that out.

now if you don't trust AddStr() well then that is a different story! I will go look at that.

here is the pertinent code... so as long as len is correct and DataLen is not exceeded then all is fine.. So..this should really be fixed.

} else {
for (; i<len && str[i]!=0; i++, index++) {
buf[index]=str[i];
}
}
for (; i<len; i++, index++) {
buf[index]=fillChar;
}

so... I think it would be better to fix AddStr() (should be fixed anyway) than have code repetition in both AddStr and AddAISStr()...

[ttlappalainen]

I'll fix AddStr(). It is currently only problematic with some PGNs, if one adds repeating fields too many times.

• Did you test current code?
• Please remove #include and use if ( len>MaxDataLen-DataLen ) len=MaxDataLen-DataLen; as I mentioned earlier.
• Can you reset PR request, so that there will be only last change. Otherwise all changes will be seen as changes.

[lsoltero]

Hi Timo,

Not sure what this means.

Can you reset PR request, so that there will be only last change. Otherwise all changes will be seen as changes.

I made the change and resubmitted the pull request.

[ttlappalainen]

If I merge PR now, then all changes will be pulled to main library. This means that version 4.21.1 is not previous. Instead one has to go down several pulls. Somewhere was way to reset that to last one so that on library one sees only one pull between 4.21.2 and 4.21.1.

• library.json is not updated
• date on changes.md is outdated

[jiauka]

... Somewhere was way to reset that to last one so that on library one sees only one pull between 4.21.2 and 4.21.1.
...
It is called rebase,

since it is 6 commit ahead, the command should be
git rebase -i HEAD~5
and has to be done at lsoltero git

[lsoltero]

thanks for the pointer..

I will do the rebase now...

Do I need to cancel the pull request and submit again? I will push some buttons here to see what happens...

[lsoltero]

so I have rebased everything into a single commit and the commit looks good. There is another commit that somehow got in there authored by timo that creates a conflict on the library version number... not sure how that got in there. and not sure how to fix that.