remove duplicate code in main\ZgatewayBT.ino #557

Floyddotnet · 2020-02-21T22:09:23Z

Based on my original PR #553 I have further developed the idea to remove duplicate code inside the Bluetooth module step by step to improve the readability and maintainability.

During this, I found a data corruption with is triggered if the white-list is set before the devices are discovered. this is addressed with 92a833e

The last two patches of this series finally reduce the need for device-tree walks and increase the overall speed if many supported ble devices are in range.

Summarizing, these changes save over 141 KB flash size (~9%) and ~600 bytes of memory

the ***Discovery functions every time create a new device unless this device is set to white-list before. as a result of this, the devices list contains the same mac multiple times with different flags

…d check

because now we have a utility function 'createOrUpdateDevice' we can globally cache this as a single flag

Floyddotnet · 2020-02-21T23:07:16Z

please wait with merge .. I found again a little issue with I want to fix before
there is a conceptional issue with the createOrUpdateDevice function ... i must rethink about it .. sorry
but if you want, you can still review the concept at all

c / c++ does not allow NULL as function parameter like modern languages

Floyddotnet · 2020-02-21T23:31:38Z

i'm done

c / c++ does not allow NULL as function parameter like modern languages

Floyddotnet · 2020-02-22T00:07:15Z

i dont know why this PR fails in travis-ci .. it build and work local

…s1d3/ESP32_BLE librarie) this makes the semaphore handling more cleaner and readable

…coveryFromList

…advertised callbacks with dummy functions for a better readability we can remove this because isDiscovered does not do a devices walk anymore

Floyddotnet · 2020-02-22T23:04:28Z

travis ci builds now works again .. I don't know what is wrong with the enums, maybe someone can give me a hint

Floyddotnet · 2020-02-23T10:23:17Z

@1technophile: I'm sure that I can merge ZgatewayBT.ino#L496 and ZgatewayBT.ino#L828 together but I didn't have a HM-10/11 for testing.
Is there a procedure for such experimental changes?
Would you have the opportunity to test it?
Or should I let it go?

Edit: For clearance, I mean the whole block

For example Floyddotnet@c181cd4

1technophile · 2020-02-27T10:11:26Z

Is there a procedure for such experimental changes?

I have it but not published.

Would you have the opportunity to test it?

Not in the following weeks unfortunately

should I let it go?

It is a good idea, we could maybe do it on a separate PR.

Another point to facilitate the maintenance of ZgatewayBT would be to add unit tests. But I didn't had the time to go in deep into this unfortunately.

Floyddotnet · 2020-02-27T10:35:18Z

Another point to facilitate the maintenance of ZgatewayBT would be to add unit tests. But I didn't have the time to go in deep into this unfortunately.

did you mean end to end tests? internally i use a combination of node.js and mocha to connect to the broker, send commands and wait for the response.

at the moment i am working on the support for LYWSD03.

i could clean up and generalize the code for the end-to-end tests a little bit and then make it available. but it will take a while as i can only work on the project on 2-3 days a week.

my current roadmap:

adjustments for active BLE connections (mostly completed yet)
- verify safe coexistence with ble scan (in progress)
- retry logic for commands (NYI)
LYWSD03 support (in progress)
EQ3 thermostatic radiator valve support (conceptional draft mostly completed)

1technophile · 2020-02-27T12:52:13Z

did you mean end to end tests?

i was thinking on pushing with some unit test runner raw service data and verify that the corresponding sensors values are the ones expected. Something that can be integrated with travis maybe by creating a separate executable script.

at the moment i am working on the support for LYWSD03.

Good news !

my current roadmap:

That's a huge contribution, thanks !

Floyddotnet · 2020-02-28T07:37:51Z

from your request for unit test i understand that you are afraid that the above changes will change the behavior.

if this assumption is correct. I have tested the changes with a Mi Jia and a LYWSD02. Both of them are transmitting discovery messages as expected, reporting temperature and humidity. I can also post the serial monitor output for you.

1technophile · 2020-02-28T21:02:11Z

Unit tests are more an enhancement that I would like to setup in a separate PR, maybe the one gathering both process_data function can be the opportunity to do it.

Do you prefer me to review this PR as is or do you want to add other commits to it ?

Floyddotnet · 2020-02-29T16:04:09Z

Do you prefer me to review this PR as is or do you want to add other commits to it ?

No, you can review the PR as it is.

Currently, I am working on LYWSD03 support and active BLE connections. It's a bit difficult to structure the code so that it can be compiled with both the ESP32 and HM10 without creating memory leaks. Also, I'm just a recreational c developer, and I occasionally have problems with some features of c / c compiler that don't occur in most high-level languages.

1technophile

First functionnal tests are ok for me, white and black list are working, devices are detected and discovered.
I will go deeper into the code the next days.

1technophile

Thanks for these important improvments!

All is working properly for me and your code learnt me some tips.

1technophile · 2020-05-14T01:38:39Z

@Floyddotnet For info I'm currently merging the code of HM10 and ESP32

Floyddotnet · 2020-05-14T17:47:15Z

👍 i will fetch this changes asap.

an update for you:

It is definitivly harder to bring LYWSD03 and EQ3 support as i expected. at first i must fix some bugs in the ble libary and then i finish the first version of my LYWSD03 implementation. but then i run in massive stabillity issues like random crashes and unexpected errors. all related with the wifi / ble coexistence mode. if i disable wifi completly, all works fine.

so i figure out that i must change some sdk-settings with are not directly available from PlatformIO or Arduino32. Then i switch to the ESP-IDF as framework, make all needed changes and abstractions (luckly not so many) and install Arduino32 as component of ESP-IDF. All my knowlege of the ESP-IDF framework configurations from other projects was partial useless because of some specials in PlatformIO. After getting it working, i realize that this don't solve all issues. so talk with the devs to get a newer version of ESP-IDF (like 4.0 or 4.1) working. this tooks a long time and solves some of the issues but brings new problems with the ble libary (with is dead).

then corona lockdown comes and stole all my free time.

this is currently the state.

i hope my english is not too bad, so that i could make everything understandable.

1technophile · 2020-05-19T23:27:14Z

Thanks for the update Peter.

Don't worry for your english, mine is worst :-)

You will find below the PR, it is a work in progress
#617

Floyddotnet added 7 commits February 21, 2020 21:00

merge duplicate code into function setWorBMac

2ffe508

rename setWorBMac to updateWorB

43056ac

add helper getDeviceByMac

89f1cc0

change updateWorB to use helper getDeviceByMac

91e4a8f

change isWhite to use helper getDeviceByMac

1256bd2

change isBlack to use helper getDeviceByMac

85ade47

change isDiscovered to use helper getDeviceByMac

e53a750

Floyddotnet requested a review from 1technophile February 21, 2020 22:10

Floyddotnet added 4 commits February 21, 2020 23:50

fix a data corruption race between MQTTtoBT and discovery-functions

92a833e

the ***Discovery functions every time create a new device unless this device is set to white-list before. as a result of this, the devices list contains the same mac multiple times with different flags

eliminate duplicate devices walk for isWhite, isBlack and isDiscovere…

f9e2fd4

…d check

eliminate duplicate devices walk for oneWhite check

5696c4a

because now we have a utility function 'createOrUpdateDevice' we can globally cache this as a single flag

secure createOrUpdateDevice agains data races on multicore esp32

118f7f3

Floyddotnet force-pushed the ble-cleanup branch from 5606c9f to 118f7f3 Compare February 21, 2020 23:01

This comment has been minimized.

Sign in to view

fix the concept of the createOrUpdateDevice

f06f66b

c / c++ does not allow NULL as function parameter like modern languages

Floyddotnet added 4 commits February 22, 2020 21:00

use nkolban Semaphore helper class (already included and used from eo…

b29f157

…s1d3/ESP32_BLE librarie) this makes the semaphore handling more cleaner and readable

merge duplicate code during createDiscovery into new helper createDis…

50ec9e0

…coveryFromList

replace excessive use of ZmqttDiscovery precompiler directive in ble …

99ba7de

…advertised callbacks with dummy functions for a better readability we can remove this because isDiscovered does not do a devices walk anymore

fix build errors found by travis-ci

49b7fd3

1technophile reviewed Mar 1, 2020

View reviewed changes

1technophile approved these changes Mar 2, 2020

View reviewed changes

1technophile added this to the v0.9.4 milestone Mar 2, 2020

1technophile added the enhancement label Mar 2, 2020

1technophile merged commit 033a583 into 1technophile:development Mar 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove duplicate code in main\ZgatewayBT.ino #557

remove duplicate code in main\ZgatewayBT.ino #557

Floyddotnet commented Feb 21, 2020 •

edited

Loading

Floyddotnet commented Feb 21, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

Floyddotnet commented Feb 21, 2020

Floyddotnet commented Feb 22, 2020

Floyddotnet commented Feb 22, 2020

Floyddotnet commented Feb 23, 2020 •

edited

Loading

1technophile commented Feb 27, 2020 •

edited

Loading

Floyddotnet commented Feb 27, 2020

1technophile commented Feb 27, 2020

Floyddotnet commented Feb 28, 2020

1technophile commented Feb 28, 2020

Floyddotnet commented Feb 29, 2020 •

edited

Loading

1technophile left a comment

1technophile left a comment

1technophile commented May 14, 2020

Floyddotnet commented May 14, 2020 •

edited

Loading

1technophile commented May 19, 2020

remove duplicate code in main\ZgatewayBT.ino #557

remove duplicate code in main\ZgatewayBT.ino #557

Conversation

Floyddotnet commented Feb 21, 2020 • edited Loading

Floyddotnet commented Feb 21, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

Floyddotnet commented Feb 21, 2020

Floyddotnet commented Feb 22, 2020

Floyddotnet commented Feb 22, 2020

Floyddotnet commented Feb 23, 2020 • edited Loading

1technophile commented Feb 27, 2020 • edited Loading

Floyddotnet commented Feb 27, 2020

1technophile commented Feb 27, 2020

Floyddotnet commented Feb 28, 2020

1technophile commented Feb 28, 2020

Floyddotnet commented Feb 29, 2020 • edited Loading

1technophile left a comment

Choose a reason for hiding this comment

1technophile left a comment

Choose a reason for hiding this comment

1technophile commented May 14, 2020

Floyddotnet commented May 14, 2020 • edited Loading

1technophile commented May 19, 2020

Floyddotnet commented Feb 21, 2020 •

edited

Loading

Floyddotnet commented Feb 21, 2020 •

edited

Loading

Floyddotnet commented Feb 23, 2020 •

edited

Loading

1technophile commented Feb 27, 2020 •

edited

Loading

Floyddotnet commented Feb 29, 2020 •

edited

Loading

Floyddotnet commented May 14, 2020 •

edited

Loading