CANopenNode / CANopenEditor

CANopen Object Dictionary Editor
GNU General Public License v3.0
141 stars 64 forks source link

Please fix the CANopenNode exporter #134

Closed mattiapesentiAGADE closed 4 days ago

mattiapesentiAGADE commented 1 month ago

I do not want to sound polemic or anything, but

it's mega annoying and frustrating that a piece of SW that comes as part of something like CANopenNode, with millions of users and contributors, is so damned bug!

Exporting with the legacy exporter -- which you are forced to use if using Zephyr/NRF SDK -- is so full of bugs that is barely usable. CO_OD.h always lacks the timeOfDay define. Every time we update the OD we have to re-add it (thanks git). CO_OD.c, if you are lucky, comes out ready to use. If you are not lucky, most of the OD entries have the wrong attributes. This makes the OD unusable: PDO mapping, multi-byte variable, all attributes are lost (but RAM storage option).

Right now, we had to switch back to the old pre-release from robincornelius.

CANopenNode commented 1 month ago

I agree, it is buggy. It is quite complex application and according to my opinion, we should rewrite the central part, etc. See also some discussion in issues or PRs. But there is a lack of time.

However, if your workflow is correct, you shouldn't have problems:

nimrof commented 1 month ago

Hi, We all need to "rant" sometimes. I would guess that your use are somewhat outside the norm, but a good first step is to make us aware of the problems.

The missing timeOfDay

CO_OD.h always lacks the timeOfDay define

So the problem is that the header should contain the same code as in CO_TIME.h and it does not?

#ifndef timeOfDay_t
typedef union {
    unsigned long long ullValue;
    struct {
        unsigned long ms:28;
        unsigned reserved:4;
        unsigned days:16;
        unsigned reserved2:16;
    };
} timeOfDay_t;
#endif

Wrong attributes

... most of the OD entries have the wrong attributes. This makes the OD unusable: PDO mapping, multi-byte variable, all attributes are lost (but RAM storage option).

By multibyte you mean string/domain/octet or just variable that takes more that one byte like int?

I have never had a problem with missing/wrong PDO mapping or when using string variables (with SDO) There was some problems with array of string and string length, but that is all fixed afik.

Can you provide some examples/samples? Are you using multi-byte variables in PDO?

PS: Sorry if I seems a defensive in my reply, " i never had problem X", just want to make it clear what i have experience with.

trojanobelix commented 1 month ago

Since 2020 TIME_OF_DAY is in time.h not in the OD-Header file. Legaycy CanopenNode V2.0 and V1.3 both have the TIME_OF_DAY declaration. So the used stack must therefore already be far behind the current one.

I use the legacy exporter without problems, maybe an XDD that produces wrong attributes would be helpful to reproduce the behaviour

@CANopenNode: Maybe some of your millions of contributors can fix some bugs in the editor. I always had the fear that you code it more or less alone ;-)

### History of TIME_OF_DAY support Author: Robin Cornelius robin.cornelius@gmail.com Date: 07.12.2017 13:33:39 Commit hash: 439af7e8a4016fdca954613815cab085ee27db61

#104 Add TIME_OF_DAY support

Add typedefs in a #ifndef to avoid an issue if/when these get added to the CO_DRIVER.h file Add specific paths for formatting TIME_OF_DAY when writing CO_OD.c/h

Currently it only initializes the default/first member of union. So only a single value is accepted. This is not perfect as it would be more human readable if we could specify milliseconds and days as separate entries but that really requires a GUI change as well.

I've not fully tested this yet but it should be good.

Author: JuPrgn julienprgn@gmail.com Date: 02.12.2020 17:27:26 Committer: GitHub noreply@github.com Commit hash: 297cebc3b5dc4c5864acaef1f837765609fbbdfc

Fix: remove TIME_OF_DAY declarations

TIME_OF_DAY is now defined on CO_TIME.h since https://github.com/CANopenNode/CANopenNode/commit/2d5d9a4eb982bfb31c65f64e5633907839999744

Author: Robin Cornelius robin.cornelius@gmail.com Date: 07.12.2020 09:51:57 Committer: GitHub noreply@github.com Commit hash: c9b084304e16bbcd978a565d8a534085a57c7c48

Merge pull request #234 from JuPrgn/patch-1

Fix: remove TIME_OF_DAY declarations

CANopenNode commented 1 month ago

Since 2020 TIME_OF_DAY is in time.h not in the OD-Header file. Legaycy CanopenNode V2.0 and V1.3 both have the TIME_OF_DAY declaration. So the used stack must therefore already be far behind the current one.

Probably this is the reason. I already forgot many details of the Legacy CanopenNode V2.0 and V1.3 versions. V4 does not require timeOfDay_t, it uses uint32_t. CANopen conformance test tool does not complain about eds file...

@CANopenNode: Maybe some of your millions of contributors can fix some bugs in the editor. I always had the fear that you code it more or less alone ;-)

Two from the three millions of the contributors voted, there is no bug. The rest of them don't want to fix it 🤔

trojanobelix commented 5 days ago

@CANopenNode I have no idea what the concrete bug is. We should close it. IMHO