Enum definition conventions in N2kTypes.h

[preeve9534]

I'm interested in using the NMEA2000 library in a windlass application. This requires extending N2kTypes.h with some relevant enums and I'd like to share this back to this project, but...

Publicly available N2K documentation makes frequent reference to data definitions in the standard. A case in point is

DD002 - Generic Status Pair

which crops up in many, many, PGNs, sometimes multiple times in a single PGN.

I note that there is an emerging tendency in N2kTypes.h for contributors to create a new enum for every application context and to my way of thinking this is a problem.

For example, at the moment N2kTypes.h contains enum tN2kBatEqSupport which uses what I guess is a mangled, abbreviated, field name as the name of the enum type and enum tN2kOnOff which provides an inaccurate (the enum is not just on and off) aliasing and (in contrast to (a)) is named so generally (to follow the style of (a) it should be tN2kGeneratorOnOff) that it pre-empts other generic use. Miserably, (a) and to a lesser extent (b), both give application specific re definitions of something that already has a standard, appropriate, consistently applied and well known name in the N2K specification snippets that are available in the public domain. In general, I notice that contributors variously abbreviate, use underscores, use camel-case, use long-names in the type declaration and then abbreviations in the member definitions, and so on.

I wonder if some guidance for contributors to N2kTypes.h would be appropriate? I'm quite happy to write something if you want.

In the case of my project, my inclination was to declare the enums I require using the N2K data definition identifier, and then to alias each definition to the (unabbreviated) N2K long name for users who feel more comfortable with a proper name: in this way the enum definition relates to the N2K standard and has all of those desirable properties mentioned above. But then I remembered that C++ only allows type aliasing and that won't work for enums. Referring back to DD002, I am unhappily left with two definitions of the same thing. It might be possible to correct this by introducing a new namespace, but this goes against library style.

enum tN2kDD002 {
                            N2kDD002_Off=0,             // [ No, Off, Disabled, Reset, "0" ]
                            N2kDD002_On=1,              // [ Yes, On, Enabled, Set, "1" ]
                            N2kDD002_Error=2,           // Error
                            N2kDD002_Unavailable=3      // [ Unavailable, Unknown ]
                          };

enum tN2kGenericStatusPair { // Alias of tN2kDD002
                            N2kGenericStatusPair_Off=0,             // [ No, Off, Disabled, Reset, "0" ]
                            N2kGenericStatusPair_On=1,              // [ Yes, On, Enabled, Set, "1" ]
                            N2kGenericStatusPair_Error=2,           // Error
                            N2kGenericStatusPair_Unavailable=3      // [ Unavailable, Unknown ]
                          };

Any thoughts, guidance would be very helpful. Thank you for all your great work on this project.

Paul

ps I think it is also possible to pick up all of the N2K specification language (in square brackets above) like this:

enum tN2kDD002 {
                            N2kDD002_No=0,             // [ No, Off, Disabled, Reset, "0" ]
                            N2kDD002_Off=N2kDD002_No,
                            N2kDD002_Disabled=N2kDD002_No,
                            N2kDD002_Reset=N2kDD002_No,
                            N2kDD002_0=N2kDD002_No,
                            N2kDD002_Yes=1,              // [ Yes, On, Enabled, Set, "1" ]
                            N2kDD002_On=N2kDD002_Yes,
                            N2kDD002_Enabled=N2kDD002_Yes,
                            N2kDD002_Set=N2kDD002_Yes,
                            N2kDD002_1=N2kDD002_Yes,
                            N2kDD002_Error=2,           // Error
                            N2kDD002_Unavailable=3,      // [ Unavailable, Unknown ]
                            N2kDD002_Unknown=N2kDD002_Unavailable
};

[ttlappalainen]

When I started project I did not had document for those general types available. On the other hand I did not got the point why to use them. Generic status pair could be usable, if you need to mix results. But where do you need to mix tN2kBatEqSupport and some other. If mixing is needed then we could have definition enum tN2kGenericStatusPair {...}; using tN2kBatEqSupport=tN2kGenericStatusPair;

In interfaces it is best to think final user. Someone has said ones that I have missing fields in N2kMessages in some messages. The reason is simple then normal user does not e.g. care about SID, so why it would be in function parameter list at all. Also I have changed order of parameters in some message so that most commonly used (I think) with those messages are first.

N2kMessages module shows simple way to handle some messages. It is easy to understand for beginner. I use it only in simple projects. Functional way works with simple messages, but makes it impossible for comples messages. Better would be to have all messages written in own objects. Then it would be easier to handle more things. I have done that for some messages, but have not published it. Sending is much easier: tN2kEngineRapid N2kMsg; N2kMsg.Speed=2500; NMEA2000.SendMessage(N2kMessage);

But receiving is ugly because of typecasting, which also makes code difficult for beginners.

I am not ready to accept your pull, since I do not yet see the point to have all those definitions tN2kDD002 , tN2kDD477, tN2kDD478 etc. I see that they would just mix beginners head. You could simply have old way definitions in N2kTypes.h and DD definitions in other header file for those who would need them. So please explain me why we would need definitions tN2kDD002 , tN2kDD477, tN2kDD478 in N2kTypes.h or at all?

There is some things, which I would like to differently now, when I have learned more about NMEA2000 and found more documentation. But I want to keep current library style and compatiblility.

[preeve9534]

I understand your argument completely (and gosh, yes, if we had only known then what we know now!) and broadly agree with how it relates to N2kMessages, but I disagree about "easy for the user" in respect of N2kTypes.h. I suspect that most users now are obtaining their information on packet content from either manufacturer released information or documents from NEMA that have leaked into the public domain. A case in point is https://www.nmea.org/Assets/20190613%20windlass%20amendment,%20128776,%20128777,%20128778.pdf.

This document, like all other I have seen, lists the fields in each PGN and their type, using the NMEA standard names for each component. There is no need for the user to "invent" or "create" anything - just copying the type names and so on gives something which will work straight away (given the appropriate type definition exists) and have consistency with the names they may encounter elsewhere in the same reference documents or others. Even the type definition can be pretty much copied from the documentation by someone with little or no programming knowledge. To my way of thinking this simplifies things for the user, makes program code directly relate-able to the associated specifications and eliminates redundancy at one fell swoop. If we always call a dog a dog (GenericStatusPair) (or a canine DD002), then that is better than inventing multiple descriptions like "hairy-thing-with-four-legs".

My other thought is that sometimes naming confuses type and application and this is a bad thing to expose a novice user to and a confusing thing for an experienced user. Your example of tN2kBatEqSupport seems to me to do this: the name reflects the use the data will be put to rather than the data type itself. As an aside, the N2K spec is very careful about this distinction with types called something like FooState and properties of the type FooStatus - I think these linguistic clues help the user to implicitly understand what is going on and help reduce unnecessary confusion.

In short, the answer to your question about why is "Because these things have a formal, defined, name which is out there in the world and visible to all users. Ignoring this doesn't help users, it patronises and confuses them."

What a lot of fun coding is!!

Paul

[ttlappalainen]

I see your point. Anyway N2kMessages should provide interface for user to make device without nearly any knowledge of NMEA2000. So there are different levels.

1. Users who just uses N2kMessages interface
2. Users who will add more functions to that interface.

Level 1 does not need to know anything about NMEA 2000 DD definitions. That we should keep as it is. This is same as I use a car. I am not interested definitions background - just need to know how to drive it.

Level 2 is a bit different, since they will read the documents and how message need to be constructed. There it would help if one would just use all with DD definitions.

But unfortunately for now I am a bit locked with current style, which I would like to maintain until something totally new - like complete message class way, which is in my mind. If you are interested about that interface, please contact me directly with email.

You could also define: enum tN2kDD477 {...}; using tN2kWindlassMonitoringEvents=tN2kDD477;

With this way your tN2kDD477 and more descriptive tN2kWindlassMonitoringEvents would be same and you can do setting between those. The only draw back is that values would be N2kDD477_... , but I do not that so bad. Anyway I would suggest shorter name like N2kWME_NoErrorsPresent so it does not matter if it would be N2kDD477_NoErrorsPresent. It is pity that C++ does not translate enums to namespaces. Then we would have value tN2kWindlassMonitoringEvents::NoErrorsPresent.

Should we put all DD definitions to new file? If there would be new interface for messages, it does not mean that it would use old definitions.

[preeve9534]

I long ago stopped worrying about the length of names of things: I'd rather wear out my fingers (maybe even not with autocomplete) than discover tN2kWaterMakerExhaust exists and if only I hadn't shortened WindlassMonitoringEvents to WME then I wouldn't have to invent a new acronym that breaks the principle I've been using to make them. I sort of think long forms are the only way forwards. As you say, a pain about C++ and its haphazard approach to namespaces.

If you like, I can split the DD definitions out into a separate file - what would you like to call it? N2kDDTypes.h? I'll also undertake to trawl my NMEA docs and add definitions for the enum types I can find that don't relate to my windlass project.

I'm happy using the "using" declaration for the long name alias (I only did the replicated definition to try and keep as close as my conscience would allow to your existing style). Although as you point out it does break the naming principle for members in the alias, but, hey-ho.

Let me know what you'd like. I will drop you an email re your message class idea.

P to name spaces.

[ttlappalainen]

I either do not worry long names, but sometimes lines just gets too long. Maybe I shoud buy wider display.

N2kTypes.h was not good choice. Should have propbaly used N2kEnumTypes.h. But I think we could use NMEA2000StdTypes.h. Then if you include that to N2kTypes.h and use using tN2kWindlassMonitoringEvents=tN2kDD477; copy below the possible values as comment: // N2kDD477_NoErrorsPresent=0, // N2kDD477_ControllerUnderVoltageCutout=1, // N2kDD477_ControllerOverCurrentCutout=2, // N2kDD477_ControllerOverTemperatureCutout=4, // N2kDD477_ManufacturerDefined=8

THen we have official name and more readable synonym tN2kWindlassMonitoringEvents. Instead of using you can also use old format typedef tN2kDD477 tN2kWindlassMonitoringEvents. Or what you think?