CANopenNode / CANopenEditor

CANopen Object Dictionary Editor
GNU General Public License v3.0
133 stars 62 forks source link

Missing definitions for subindex in CANOpenNode export #128

Closed C0ntroller closed 1 month ago

C0ntroller commented 3 months ago

Hello,

When comparing old code that was generated using v0.6 of libedssharp and my now newly generated one using this fork, I noticed, that there are definitions missing for accessing fields at a subindex.

What I mean with this, is that for example I have an array at 0x2121 (lets call it Test_Array). Every one of the 14 fields inside (excluding the max sub-index entry) are properly named and defined (lets say, they are named Test_x where x is the index in hex). But when exporting it, to directly access them, I would need to hardcode their sub-index. For example:

// For the 13th element
/// The 0xd here must be hardcoded, as no definition for "Test_d" is to be found
OD_ENTRY_H2121_Test_Array[0xd] = 1;
// In previous versions
CO_OD_RAM.Test_Array[ODA_Test_Array_Test_d] = 1

This change seems to already had happened at v0.8 of the original project.

Would it be possible to get these definitions back? It does not have to be exactly the same way, but any way would be great.

If not, what is the expected way to do this? Just hardcoding it?

nimrof commented 3 months ago

Hi @C0ntroller, There is something similar in the current version, but it is not hex as far as i can see

#define OD_1003_0_preDefinedErrorField_maxSubIndex          0
#define OD_1003_1_preDefinedErrorField_standardErrorField   1
#define OD_1003_2_preDefinedErrorField_standardErrorField   2
#define OD_1003_3_preDefinedErrorField_standardErrorField   3
#define OD_1003_4_preDefinedErrorField_standardErrorField   4
#define OD_1003_5_preDefinedErrorField_standardErrorField   5
#define OD_1003_6_preDefinedErrorField_standardErrorField   6
#define OD_1003_7_preDefinedErrorField_standardErrorField   7
#define OD_1003_8_preDefinedErrorField_standardErrorField   8
#define OD_1003_9_preDefinedErrorField_standardErrorField   9
#define OD_1003_10_preDefinedErrorField_standardErrorField  10

If elements are named this is generated also and that is kind of what you use, but it is name instead of raw index

/*1011, Data Type: UNSIGNED32, Array[4] */
#define OD_restoreDefaultParameters_idx                     0x1011
#define OD_restoreDefaultParameters                         CO_OD_RAM.restoreDefaultParameters
#define ODL_restoreDefaultParameters_arrayLength            4
#define ODA_restoreDefaultParameters_restoreAllDefaultParameters 0
#define ODA_restoreDefaultParameters_restoreCommunicationDefaultParameters 1
#define ODA_restoreDefaultParameters_restoreApplicationDefaultParameters 2
#define ODA_restoreDefaultParameters_restoreManufacturerDefinedDefaultParameters 3

Would it be possible to get these definitions back? It does not have to be exactly the same way, but any way would be great. I think so just make a pr.

What do you think @CANopenNode ?

If not, what is the expected way to do this? Just hardcoding it?

I mostly use the named index and hardcode or use the named version for subindex, but Janez could maybe tell the intended way

CANopenNode commented 3 months ago

This is v1.3 exporter. As I remember, that was not removed, but names of subelements in the array must be unique, otherwise "ODA_" macros will not be generated.

C0ntroller commented 3 months ago

Hi @C0ntroller, There is something similar in the current version, but it is not hex as far as i can see

#define OD_1003_0_preDefinedErrorField_maxSubIndex          0
#define OD_1003_1_preDefinedErrorField_standardErrorField   1
#define OD_1003_2_preDefinedErrorField_standardErrorField   2
#define OD_1003_3_preDefinedErrorField_standardErrorField   3
#define OD_1003_4_preDefinedErrorField_standardErrorField   4
#define OD_1003_5_preDefinedErrorField_standardErrorField   5
#define OD_1003_6_preDefinedErrorField_standardErrorField   6
#define OD_1003_7_preDefinedErrorField_standardErrorField   7
#define OD_1003_8_preDefinedErrorField_standardErrorField   8
#define OD_1003_9_preDefinedErrorField_standardErrorField   9
#define OD_1003_10_preDefinedErrorField_standardErrorField  10

If elements are named this is generated also and that is kind of what you use, but it is name instead of raw index

/*1011, Data Type: UNSIGNED32, Array[4] */
#define OD_restoreDefaultParameters_idx                     0x1011
#define OD_restoreDefaultParameters                         CO_OD_RAM.restoreDefaultParameters
#define ODL_restoreDefaultParameters_arrayLength            4
#define ODA_restoreDefaultParameters_restoreAllDefaultParameters 0
#define ODA_restoreDefaultParameters_restoreCommunicationDefaultParameters 1
#define ODA_restoreDefaultParameters_restoreApplicationDefaultParameters 2
#define ODA_restoreDefaultParameters_restoreManufacturerDefinedDefaultParameters 3

I can't really duplicate that.

Just so we mean the same thing, here's my workflow:

After that I cant find anything like #define OD_1003_1_preDefinedErrorField_standardErrorField 1 in the generated header (nor the .c but thats expected).

Am I at fault here?

This is v1.3 exporter. As I remember, that was not removed, but names of subelements in the array must be unique, otherwise "ODA_" macros will not be generated.

In workflow above this this should be the case for the Store parameters field at 0x1010. But I still can't find the definitions.

nimrof commented 3 months ago

Am I at fault here?

maybe, is v4 exporter selected in preference? If yes, select the other one and try again image

CANopenNode commented 3 months ago

V4 does not provide those macros. It works differently.

C0ntroller commented 2 months ago

Am I at fault here?

maybe, is v4 exporter selected in preference? If yes, select the other one and try again image

Yes, this works, thank you.

V4 does not provide those macros. It works differently.

But what is the suggested way for not using magic numbers in code?

Also, I know this is off topic, but I guess you are way more experienced than me using CANOpenNode: Should I rather use an "old" version rather than the current state (master says it's version 4.0)?

CANopenNode commented 2 months ago

Make your own definitions? See also CANopenDemo.

nimrof commented 1 month ago

Closing as non-active