luebbe / homie-node-collection

Collection of Node implementations for the Homie-ESP8266 library
MIT License
26 stars 12 forks source link

Pingnode #11

Closed jimtng closed 4 years ago

jimtng commented 4 years ago

Changes:

I deliberately changed the method's name from setMicrosecondsToMetersFactor to setMicrosecondsToMeter

Code that calls setMicrosecondsToMetersFactor will need to rename it to setTemperature

luebbe commented 4 years ago

Hi Jim, Very nice! Do you know how to mark setMicrosecondsToMetersFactoras deprecated? Maybe that would be an option instead of dropping the method.

jimtng commented 4 years ago

Do you know how to mark setMicrosecondsToMetersFactoras deprecated?

It seems that "deprecated" is only available in C++14. There are some suggestions on stackoverflow about implementing "deprecated" in earlier versions with macros.

luebbe commented 4 years ago

OTOH would it really hurt, if you just remove the function? I guess that right now maybe 3-4 people are using the pingnode, because it was just recently submitted by @ArdKuijpers.

jimtng commented 4 years ago

I can remove it if you're happy with that. It's just a single line.

I have also changed the constructor to include the node name. It was previously hard coded to "RCW-0001".

In the SensorNode, the arguments are called "name, type"

SensorNode::SensorNode(const char *name, const char *type)
    : HomieNode(name, type, "sensor")

However, if you looked inside HomieNode (v3 - develop branch)

  HomieNode(const char* id, const char* name, const char* type, bool range = false, uint16_t lower = 0, uint16_t upper = 0, const HomieInternals::NodeInputHandler& nodeInputHandler = [](const HomieRange& range, const String& property, const String& value) { return false; });

The first argument is id and the second is name, the third is type

So starting from SensorNode and everything else that derives from it, including PingNode is calling it the wrong name. The "name" is passed to HomieNode as the "id" and the 'type" is passed to HomieNode as "name", and the actual "type" is hardcoded as "sensor".

Fixing this requires fixing everything... but that's probably for a different PR

luebbe commented 4 years ago

Please remove it. I guess we'll be fine with that.

Good catch. I'll have a look at the id/name issue. IIUC, renaming the arguments in SensorNode properly should fix it.

jimtng commented 4 years ago

Please remove it. I guess we'll be fine with that.

Done.

luebbe commented 4 years ago

Thanks! BTW: does the info about pingnode in README need an update too?

jimtng commented 4 years ago

BTW: does the info about pingnode in README need an update too?

The README is actually still valid as it is. It talks about setTemperature() which remains the same.

ArdKuijpers commented 4 years ago

@jimtng Great work. I did some refactoring during the development myself and the SetMicroSecondsToMeterFactor should have been removed but was still there. Good you found it.