CANopenNode / CANopenSocket

Obsolete repository, replaced by CANopenDemo and CANopenLinux
Apache License 2.0
208 stars 119 forks source link

Fix some compilation error #17

Closed jdascenzio closed 4 years ago

jdascenzio commented 4 years ago

This is some various compilation error I find.

geoffrey-vl commented 4 years ago

@jdascenzio Great that you get this to build using the new CANOpenNode master. May I suggest to add a better description of what you did, and why you had to make some of the adjustments, because that's missing until I started looking into your commits. For example, in your first commit I noticed you've updated the CANOpenNode submodule to https://github.com/CANopenNode/CANopenNode/commit/b92b4bb5b1fcb41c2291fc3f76330bb0c981e20e. That's a big change because its using the split-driver for what I know, so its better to put that in the description too. It will help maintainers to review your changes.

Also you may want to split your changes over 2 (or more) pull requests. Updating the submodule version should be a different pull request than one where you fix "CO_command: fix memset overflow" and other small errors and warnings. It will help keeping the git history clean.

Some comments:

"submodule CANopenNode update" (4c39b47e236f095232c6173e63876c2491aac1e3) ok but a better description as to what version your updating to may explain a bit more.

"CO_command: fix memset overflow" (c15d6b823c5b2f1e2fe85d846333a3ba9d50ea3c) only changes once the use of sizeof(), but in fact the file has used it multiple times so your changes are not very coherent here. I think these changes should not be part of your pull request, but maybe a different pull request with small changes. Also its a bit about personal taste, but we could discus this apart from updating the canopennode submodule.

"main: use reference of CANdevice0Index" (8ee3a12016646571781eb0c3c99130c984899c7f) seems to be OK, but its should be named pointer instead of reference.

"CO_command: fix a gcc warning" (1a1913fc3aedea29ccdc910f8bc0eff5d57d17bb), "main: remove an used variable" (1ea1191bdafd85080d02b824cffa5eaff47b7a1a) is ok, but should not be part of upgrading canopennode to split-driver. It's okay to put this in different pull request with small fixes.

"CO_LSS_master: fix compile error, use identity" (fa95b55080dfa19092fe63789e432a4bae20139b) is ok and is also related to updating to split-driver.

@martinwag or @CANopenNode what's you view on this. Current CANopennode submodule is quit old and could use an update to the latest master (or v1.3). Maybe we may branch CANOpenSocket as well with V1.x-master and (split-driver) master?

geoffrey-vl commented 4 years ago

One other note: how did you tackle the fact that canopend/CO_time.h and CO_TIME.h are both provided, and both have the same guards: #ifndef CO_TIME_H. I can't find any related changes in your PR.

CANopenNode commented 4 years ago

I verified this pull-request and it seems OK. I added some extra minor updates. CANopenNode is now at "v1.3-master". It is before the "split-driver", it uses old "stack/socketCAN" driver.

It seems it works OK now. There is not much changes from previous versions.

However, canopend will be removed from this repo, because canopend with the same functionality is now available inside CANopenNode itself.

I plan to keep CANopenSocket for different Linux examples. CANopenNode will be updated to the latest version after canopend will be fully transferred.

geoffrey-vl commented 4 years ago

Any reason why you have tracing disabled? Because that's exactly what's not working on my side.

EDIT:

I've also noticed that CANopenNode v1.3-master has an "include loop" in CO_trace.h line 34: image

it's already fixed in master, I guess it should actually keep the previously code:

#include "CO_driver.h"
#include "CO_SDO.h"
CANopenNode commented 4 years ago

Yes. Fixed both repos.