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 initial support for Owon AC201 Thermostat [02] #3423

Merged
merged 8 commits into from Nov 14, 2020

Conversation

SwoopX
Copy link
Collaborator

@SwoopX SwoopX commented Oct 15, 2020

  • Add initial support for Owon AC201 Thermostat
  • Introduce fan_control.cpp to accomodate AC fan control for thermostats
  • Introduce RConfigFanMode to set AC fan mode
  • Introduce RConfigSwingMode to set AC louvers position

@SwoopX SwoopX linked an issue Oct 15, 2020 that may be closed by this pull request
Copy link
Member

@manup manup left a comment

Choose a reason for hiding this comment

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

No errors just a few comments :)

thermostat.cpp Outdated
Comment on lines 617 to 624
QString mode_set;

mode_set = QString("fully closed");
if ( mode == 0x01 ) { mode_set = QString("fully closed"); }
if ( mode == 0x02 ) { mode_set = QString("fully open"); }
if ( mode == 0x03 ) { mode_set = QString("quarter open"); }
if ( mode == 0x04 ) { mode_set = QString("half open"); }
if ( mode == 0x05 ) { mode_set = QString("three quarters open"); }
Copy link
Member

Choose a reason for hiding this comment

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

The code has no errors, but for consistency please stick to camelCase — ugly, but well that's Qt style :)
Also use QLatin1String for all static strings, since it as no extra cost, only the pointer and length is stored. QString("abcd") always results in extra allocations on the heap and utf-8 conversion, not dramatic but it piles up.

Since mode has only one value, else if prevents evaluating every statement.

    QString  modeSet = QLatin1String("fully closed");
    
    if      ( mode == 0x01 ) { modeSet = QLatin1String("fully closed"); }
    else if ( mode == 0x02 ) { modeSet = QLatin1String("fully open"); }
    else if ( mode == 0x03 ) { modeSet = QLatin1String("quarter open"); }
    else if ( mode == 0x04 ) { modeSet = QLatin1String("half open"); }
    else if ( mode == 0x05 ) { modeSet = QLatin1String("three quarters open"); }

fan_control.cpp Outdated
QDataStream stream(&task.zclFrame.payload(), QIODevice::WriteOnly);
stream.setByteOrder(QDataStream::LittleEndian);

stream << (quint16) attrId;
Copy link
Member

Choose a reason for hiding this comment

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

Just a note for the future: We don't have a coding style yet, but always use c++ named casts instead old C style casts.

static_cast<quint16>(attrId)

This might seam a bit superfluous at first. But soon the next generation code will get proper types for certain values like ZclAttributeId_t, EndpointId_t, etc to prevent common mistakes.

The named casts help the compiler to figure out invalid type conversions at compile time to prevent accidental errors.

For example:

ZclAttributeId_t attrId(0x0077);

stream << (quint8) attrId;              // programmer mistake won't be detected by compiler
stream << static_cast<quint8>(attrId);  // compile error no valid conversion from ZclAttributeId_t to quint8
stream << static_cast<quint16>(attrId); // ok
stream << attrId;                       // ok, because operator<< for QDataStream

So once we have the new types we only need to change the function signature:

addTaskFanControlReadWriteAttribute(TaskItem &task, uint8_t readOrWriteCmd, uint16_t attrId, uint8_t attrType, uint32_t attrValue, uint16_t mfrCode)

to

addTaskFanControlReadWriteAttribute(TaskItem &task, uint8_t readOrWriteCmd, ZclAttributeId_t attrId, ZclDatatypeId_t attrType, uint32_t attrValue, ZclManufacturerCode_t mfrCode)

If we already use C++ named casts we get a hole lot of error checking without needing to rewrite all those C style casts, searching for them is also quite hard/impossible.

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.

OWON AC201 Thermostat (IR Blaster)
2 participants