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 HEX string type and option to disable auto-subscribe #134

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

promd
Copy link

@promd promd commented Nov 9, 2021

This adds an option to the config that disables the automatic notification subscription.
It also introduces an "Hex-String" type, in case you don't want to sent integer representations.

Copy link
Owner

@shmuelzon shmuelzon left a comment

Choose a reason for hiding this comment

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

Hey,

Thanks for the pull request!
Please remove the addition and reversion of the gzip command, no need to clutter the git history, and just have the two commits in the pull request.
Should the hex option also be documented somewhere?

main/config.c Outdated
Comment on lines 180 to 189
/* Not defined, to subscribe is default */
if (cJSON_IsTrue(subscribe))
{
return true;
}
else if (cJSON_IsFalse(subscribe))
{
return false;
}
return true;
Copy link
Owner

Choose a reason for hiding this comment

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

This can all be changed to simply

return !cJSON_IsFalse(subscribe);

main/ble2mqtt.c Outdated
Comment on lines 459 to 460
if (properties & (CHAR_PROP_NOTIFY | CHAR_PROP_INDICATE))
{
Copy link
Owner

Choose a reason for hiding this comment

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

Please just change this condition to:

    if (properties & (CHAR_PROP_NOTIFY | CHAR_PROP_INDICATE) &&
        config_ble_characteristic_should_subscribe(uuidtoa(characteristic_uuid)))
    {

And leave the below as it was.

README.md Outdated
@@ -256,6 +256,8 @@ configuration:
http://www.bluetooth.org. Each characteristic can include a `name` field which
will be used in the MQTT topic instead of its UUID and a `types` array
defining how to parse the byte array reflecting the characteristic's value.
The `subscribe` attribute can be set to "false" if you don't want to automatically
subscribe for Notifications and Indications on a Characteristic.
Copy link
Owner

Choose a reason for hiding this comment

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

No need to capitalize Notifications, Indications nor Characteristic.

@shmuelzon shmuelzon force-pushed the master branch 2 times, most recently from 7c19828 to 6ebee2a Compare October 11, 2024 08:12
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.

2 participants