Added handler for PGN127252, minor syntax fix for PGN129802, according to canboat definitions, PGN129285 had a bit alignment bug, this is now fixed and type definitions had a minor improvement.

[aydosj]

[ttlappalainen]

Heave PGN 127252 has totally 5 fields and you have set them all otherwise some devices does not read it righ. Use NA value, you you do not provide data.

129285:

• Fix: N2kMsg.AddByte(0xE0 | (SupplementaryData & 0x03)<<2 | (NavDirection & 0x03));
• For route name use N2kMsg.AddVarStr(RouteName);
• On parser provide also name buffer length and use it.
• You are using GetStr, but you should use GetVarStr - have you actually tested this?
• PGN also carries WP information. Are you not interested to set/parse it?

[aydosj]

Thanks Timo,

Please see answers inline.

On Thu, 2022-08-25 at 02:31 -0700, Timo Lappalainen wrote:

Heave PGN 127252 has totally 5 fields and you have set them all otherwise some devices does not read it righ. Use NA value, you you do not provide data. I only have github.com/canboat project as reference, it shows 3 fields. This could be extended by someone with more knowledge. Happy to remove it otherwise. 129285: The set function was already there I and saw issues that I thought I would fix. The set function was already there I and saw issues that I thought I would fix.

• Fix: N2kMsg.AddByte(0xE0 | (SupplementaryData & 0x03)<<2 | (NavDirection & 0x03)); canboat shows 4 reserved bits, hence I chose 0xF0. Please help me understand why you would set 0xE0?

• For route name use N2kMsg.AddVarStr(RouteName); The original implementaion did not use AddVarStr. I was not trying to change this part. Happy to fix it though.

• On parser provide also name buffer length and use it. Will do.

• You are using GetStr, but you should use GetVarStr - have you actually tested this? Yes, but it was a limited test case. I can see the problem, will change to  GetVarStr

• PGN also carries WP information. Are you not interested to set/parse it? I don't really need the parser.  I saw it was missing and decided to implement just to contribute. Half way through I realised the canboat reference I had was not enough to confidently complete the implementation. So I left it as a stub hoping someone else would be able to complete it in the future. I am happy to remove it if you prefer.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

[ttlappalainen]

• Add for PGN 127252 setter:

N2kMsg.Add2ByteUDouble(N2kDoubleNA, 0.01); // Field 3, Delay
N2kMsg.AddByte(0xff); // Field 4, Delay source, Field 5, Reserved
N2kMsg.AddByte(0xff); 
N2kMsg.AddByte(0xff);

• Wrong on my fix too. Must be N2kMsg.AddByte(0xE0 | (SupplementaryData & 0x03)<<3 | (NavDirection & 0x07)); Field 5: nav. direction 3 bits, Field 6: supplementary route/WP data available 2 bits, Field 7: reserved 3 bits Also parser is wrong and must be:

  NavDirection = (tN2kNavigationDirection)(vb & 0x07);
  SupplementaryData = (tN2kTrueFalse)( (vb>>3) & 0x03 );
  

• Also you have to cancel changes for tN2kNavigationDirection in N2kTypes.h. Original definition was correct.

As you have done parsers, just leave them, but test they work. Do simple program sending data and other one reading data. Or extend e.g., MessageSender and DataDisplay2 to test them.

And in general every code you add or change must be carefully tested. Do not add any untested code just for fun. And with test I do not mean test cases. If you write e.g., parser wrong, you will probably write also test wrong. It must be tested with real data.

[ttlappalainen]

There are still other fixes waiting, so I can not merge this.

[aydosj]

Hi Timo,

I will send another request later when I come back to this.

On Mon, 26 Sept 2022 at 20:56, Timo Lappalainen @.***> wrote:

There are still other fixes waiting, so I can not merge this.

— Reply to this email directly, view it on GitHub https://github.com/ttlappalainen/NMEA2000/pull/280#issuecomment-1257853152, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKN7XNALLR5ZN2DTTXF2XU3WAF6OLANCNFSM57SGWXCA . You are receiving this because you authored the thread.Message ID: @.***>

[ttlappalainen]

I have applied most of these to new release, which I'll publish in few days. So you can remove this PR, since it would conflict new release anyway.

[aydosj]

Ok

On Sat, 1 Oct 2022, 12:22 am Timo Lappalainen, @.***> wrote:

I have applied most of these to new release, which I'll publish in few days. So you can remove this PR, since it would conflict new release anyway.

— Reply to this email directly, view it on GitHub https://github.com/ttlappalainen/NMEA2000/pull/280#issuecomment-1263641616, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKN7XNHIESPKZHTNDNPHYA3WA3ZQ3ANCNFSM57SGWXCA . You are receiving this because you authored the thread.Message ID: @.***>

[aydosj]

Implemented offline