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

SensorSignal reports 0 if MY_SIGNAL_REPORT_ENABLED not defined in sketch #504

Closed
liwenyip opened this issue Apr 18, 2020 · 2 comments
Closed
Labels
Projects

Comments

@liwenyip
Copy link
Contributor

liwenyip commented Apr 18, 2020

Hi,

Thank you @user2684 for creating and maintaining this project, it is AWESOME

Nodemanager v1.8
MySensors v2.3.2
Arduino Pro Mini 3.3V

I've noticed that you have to #define MY_SIGNAL_REPORT_ENABLED at the top of the sketch, otherwise SensorSignal always reports zero.

MY_SIGNAL_REPORT_ENABLED is defined at the top of NodeManager/sensors/SensorSignal.h, but that's too late - it has to be defined before thetransportGetSignalReport function (MySensors/core/MyTransport.cpp:1092) is compiled, otherwise that function is compiled to always return zero.

Looking for your thoughts on solution before I submit a PR:

  1. Put a comment in the template sketch:
//#include <sensors/SensorSignal.h>
//SensorSignal signal;
// NB uncomment #define MY_SIGNAL_REPORT_ENABLED above for this to work
  1. Instead of defining MY_SIGNAL_REPORT_ENABLED in SensorSignal.h, throw a compiler error if it has not already been defined:
#ifndef MY_SIGNAL_REPORT_ENABLED
#error "MY_SIGNAL_REPORT_ENABLED must be defined in the sketch, before #include <MySensors_NodeManager.h>"
#endif
  1. Any other more elegant ideas?

Thanks in advance

@user2684 user2684 added the bug label May 3, 2020
@user2684 user2684 added this to Triage in v1.9 via automation May 3, 2020
@user2684
Copy link
Contributor

user2684 commented May 3, 2020

Thanks for reporting it, never noticed this thing. Wonder why all my sensors are correctly reporting the signal without any change. Do you get a 0 signal reported instead? If confirmed, I'd go for option 2, seems to me the most clean approach. Thanks!

@user2684
Copy link
Contributor

Hi, I was finally able to reproduce the issue, seems related to the latest version of MySensors library this is why was not showing up straight away. Fixed through #517 . Thanks!

v1.9 automation moved this from Triage to Done May 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
v1.9
  
Done
Development

No branches or pull requests

2 participants