Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CAN transport layer #1488

Open
wants to merge 14 commits into
base: development
Choose a base branch
from

Conversation

AdamSlowik
Copy link

Add new transport layer.
Uses github.com/coryjfowler/MCP_CAN_lib maintained by @coryjfowler.

@AdamSlowik
Copy link
Author

Is there some simple way of running those jenkins tests on local machine? Perhaps with some docker assistance?

@AdamSlowik
Copy link
Author

Details of build errors are no longer available. Can someone trigger jenkins job?

@virtual-maker
Copy link
Contributor

@AdamSlowik

Hi Adam, thank you for your extension of MySensors to include the CAN bus transport layer. I am glad that you are also so persistent in satisfying the toll gate.

The "Butler" step is passed now, perfect!
Unfortunately you are now at the same point as in PR #1522.

Strangely enough, the last two PR #1524 and PR #1525 have successfully passed the toll gate. I therefore wonder what is different about your PR.

My suspicions for possible causes are:

  1. your development branch is now 6 commits behind the development of MySensors repo. You could try to rebase your branch onto your upstream development.
  2. the successful PR's consisted of only one commit. So you could try to squash your commits and force push again.

See also my comments here:
1515#issuecomment-1030930615
1515#issuecomment-1030939724

@virtual-maker
Copy link
Contributor

@AdamSlowik Yes, after squash toll gate looks different.
I found 2 warnings in Jenkins from your new CAN code. I hope when you fix both, toll gate may pass.
Jenkins log

MySensors/hal/transport/CAN/MyTransportCAN.cpp:228:21: warning: unused variable 'to' [-Wunused-variable]
   long unsigned int to = (rxId & 0x0000FF00) >> 8;
                     ^~
MySensors/examples/CANSwitch/CANSwitch.ino:96:17: warning: unused variable 'sentValue' [-Wunused-variable]
  static uint8_t sentValue=2;
                 ^~~~~~~~~

Good luck 👍

AdamSlowik added a commit to AdamSlowik/MySensors that referenced this pull request Jun 29, 2022
@AdamSlowik
Copy link
Author

According to:

https://ci.mysensors.org/blue/rest/organizations/jenkins/pipelines/MySensors/pipelines/MySensors/branches/PR-1488/runs/15/nodes/274/steps/290/log/?start=0

There are many warnings, but not related to My changes. Is there any other log file? Why other branches do not fail on those warnings?

@AdamSlowik
Copy link
Author

@virtual-maker could You take a look at tool gate logs? Perhaps I missed something.

@virtual-maker
Copy link
Contributor

@AdamSlowik
Looks like this is the same problem with this new PJON driver stuff.
Toll gate (STM32F1 - Tests) — Warnings found
Details

In the full Jenkins log
I found this in lines around line# 8620:

In file included from /var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1488/workspace/MySensors/hal/transport/PJON/driver/PJON.h:62,
                 from /var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1488/workspace/MySensors/MySensors.h:397,
                 from /var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1488/workspace/MySensors/tests/Arduino/sketches/pjon_transport/pjon_transport.ino:24:
/var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1488/workspace/MySensors/hal/transport/PJON/driver/PJONDefines.h: In static member function 'static void PJONTools::parse_header(const uint8_t*, PJON_Packet_Info&)':
/var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1488/workspace/MySensors/hal/transport/PJON/driver/PJONDefines.h:415:31: warning: 'void* memset(void*, int, size_t)' clearing an object of non-trivial type 'struct PJON_Packet_Info'; use assignment or value-initialization instead [-Wclass-memaccess]
  415 |   memset(&info, 0, sizeof info);
      |                               ^
/var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1488/workspace/MySensors/hal/transport/PJON/driver/PJONDefines.h:207:8: note: 'struct PJON_Packet_Info' declared here
  207 | struct PJON_Packet_Info {
      |        ^~~~~~~~~~~~~~~~

There was a discussion in PR #1520 to exactly the same warnings in PJONDefines.h.
I assume that this memset(&info, 0, sizeof info); is not best practice and should be replaced.

virtual-maker pushed a commit to virtual-maker/MySensors that referenced this pull request Jul 29, 2022
@nschurando
Copy link

Great work! I'm interested in using can as a transport later as well. Leaving message to be notified of any progress. Let me know if I can contribute in some way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants