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

Added support for sensors #7

Merged
merged 5 commits into from
Jun 29, 2017
Merged

Added support for sensors #7

merged 5 commits into from
Jun 29, 2017

Conversation

StefanKrupop
Copy link
Contributor

This change allows defining sensors in the RDMINIT struct. When a defined sensor is queried via RDM, a callback function is called with the sensor number as input. The function can then set the current, minimum, maximum and recorded value.

Copy link
Contributor

@peternewman peternewman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, mostly from the perspective of the E1.20 spec. Have you tried running the new code against the OLA RDM tests?

WRITEINT(_rdm.packet.Data , E120_MANUFACTURER_LABEL);
WRITEINT(_rdm.packet.Data+ 2, E120_DEVICE_MODEL_DESCRIPTION);
WRITEINT(_rdm.packet.Data+ 4, E120_DEVICE_LABEL);
WRITEINT(_rdm.packet.Data+ 6, E120_SENSOR_DEFINITION);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's not much point declaring these if the device doesn't have any sensors (i.e. sensorsLength <= 0).

nackReason = E120_NR_SUB_DEVICE_OUT_OF_RANGE;
} else {
uint8_t sensorNr = _rdm.packet.Data[0];
if (sensorNr >= _initData->sensorsLength) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also check that sensorNr < 0xff (all sensors) either here, or at init time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... sensorsLength is defined as uint8_t, so the highest possible value would be 0xff anyway. As the "if" here checks for larger or equal, it would return E120_NR_DATA_OUT_OF_RANGE when the user would have defined 255 sensors and sensorNr is 0xff.
Maybe it's too late here (3:38 am), but I think the logic works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's sort of my point, you can only have 254 sensors, but you could technically pass 255 into the system. It probably really wants to be at init time, but you ought to ignore a sensor 255, as that's reserved for "all sensors".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but you ought to ignore a sensor 255

Isn't this exactly what is happening? If the user should have passed 255 sensors, sensorsLength would also be 255. If then sensor 255 is requested, the library replies with E120_NR_DATA_OUT_OF_RANGE instead of the sensor data, effectively ignoring it.
Also, for any smaller number of defined sensors 255 will result in E120_NR_DATA_OUT_OF_RANGE.

Sorry, but I still do not see the case where this is problematic...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep fair point. I think it needs trapping/overwriting earlier though as otherwise devInfo->sensorCount will be populated by it, which can't be valid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, double-checking the standard, it says "Valid sensor numbers are in the range from 0x00 – 0xFE. The sensor number 0xFF is used to address all sensors." and "When a device or sub-device is fitted with a single sensor, it would return a value of 0x01 for the Sensor Count. This sensor would then be addressed as Sensor Number 0x00 when using the other sensor-related parameter messages.".

So you can have 255 sensors, which are addressed as 0 to 254.

nackReason = E120_NR_SUB_DEVICE_OUT_OF_RANGE;
} else {
uint8_t sensorNr = _rdm.packet.Data[0];
if (sensorNr >= _initData->sensorsLength) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again 0xff.

}
}
} else if (CmdClass == E120_SET_COMMAND) {
// Unexpected set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not unexpected, SET lets you clear/reset the recorded info. Here 0xff (all) clears for all sensors.

@@ -344,14 +345,15 @@ int random255();

// Initialize or reinitialize the DMX RDM mode.
// The other values are stored for later use with the specific commands.
void DMXSerialClass2::init(struct RDMINIT *initData, RDMCallbackFunction func, uint8_t modePin, uint8_t modeIn, uint8_t modeOut)
void DMXSerialClass2::init(struct RDMINIT *initData, RDMCallbackFunction func, RDMGetSensorValue sensorFunc, uint8_t modePin, uint8_t modeIn, uint8_t modeOut)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break people's existing code, you should probably either stick it at the end, or do a wrapper version of the old prototype which calls the new one with a NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, added init with "old" signature in header

- Only report E120_SENSOR_DEFINITION and E120_SENSOR_VALUE when sensors are defined
- Fixed comment for settings sensor values
@StefanKrupop
Copy link
Contributor Author

Yes, with some other (unrelated) fixes, I can get:
367 / 426 tests run, 367 passed, 0 failed, 0 broken
Tested with "OLA Responder Tests Version 0.10.3".

@mathertel
Copy link
Owner

I just had the chance to check this pull request.
Thanks for the contribution.

@mathertel mathertel closed this Jun 29, 2017
@mathertel mathertel reopened this Jun 29, 2017
@mathertel mathertel merged commit 091abda into mathertel:master Jun 29, 2017
@peternewman
Copy link
Contributor

@saiteja9429 you'd be better off starting a new issue:
https://github.com/mathertel/DmxSerial2/issues/new

I suspect it's most likely to be an electrical/electronic issue. Does DMX work to the device?

@ghost
Copy link

ghost commented Aug 18, 2018

I have discovered many small issues with DMXcat, not sure if it is in the phone app or in the DMXcat unit's software or hardware.

@saiteja9429
Copy link

saiteja9429 commented Aug 19, 2018 via email

@peternewman
Copy link
Contributor

Do you have access to a DMXcat @Jabadabado ?

@saiteja9429 there's a chance it could be this bug #10 . I've done an untested fix here #13

Have you wired up the direction pin of your RS485 transciever to the Arduino? By default it should be connected to pin 2, or you can override it by changing init() :
https://github.com/mathertel/DmxSerial2/blob/master/src/DMXSerial2.h#L178-L188

@saiteja9429
Copy link

saiteja9429 commented Aug 20, 2018 via email

@saiteja9429
Copy link

saiteja9429 commented Aug 20, 2018 via email

@saiteja9429
Copy link

saiteja9429 commented Aug 20, 2018 via email

@peternewman
Copy link
Contributor

That's your problem @saiteja9429 :

RE,DE - GND

Your system can't switch between Tx and Rx. You want the MAX485 connection parts of this or similar (I think you could get away without R1, I believe that's just to pull it up if the Arduino crashes):
https://www.instructables.com/id/Arduino-RDM-Relay/

In terms of the manufacturer and RDM UID, see the notes here:
https://github.com/mathertel/DmxSerial2/blob/master/src/DMXSerial2.cpp#L221-L236

That manufacturer ID is registered to Mathertel:
http://rdm.openlighting.org/manufacturer/display?manufacturer=2439

There are also a group reserved for prototyping:
http://rdm.openlighting.org/manufacturer/list

@saiteja9429
Copy link

saiteja9429 commented Aug 29, 2018 via email

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.

None yet

4 participants