search

I-Connect / NukiBleEsp32

GNU General Public License v3.0
35 stars 20 forks source link

Set the correct payload size when using parameter 'nameSuffix' for lo… #40

Closed Kelesys62 closed 6 months ago

Kelesys62 commented 1 year ago

Hello, this is my first contribution on GitHub, I hope it may help fixing the issue I found for your library. If something wrong, don't hesitate to let me know. Regards. Kelesys

Bascy commented 7 months ago

Finally getting around to handling your PR ..... we have a question:

What is the problem you are trying to solve here? [Edit} Ah we found it! Issue #39 (Pro tip for creating pull request: include the issue number with hash sign in the comments ;))

payload is an array of length 100, but you are limiting it to 20 chars. Are the remaining 80 chars still needed?

Bascy commented 7 months ago

Ok, we now understand what you are trying to achieve with the modification. Functional it is correct, but we don;t like that you change the interface of the method, as this will make this a breaking change.

Please restore the namesuffixLen parameter, and modify the code to only use max 20 chars if namesuffixlen is larger then 20. (btw, please check if a /0 terminator is needed in the payload)

Thanks for your contribution!

Kelesys62 commented 7 months ago

Hello, Yes you are right, I didn't think about breaking changes introduced by my fix. 😅 I will update it as soon as I get my nuki back to avoid pushing untested code 😉 Thanks you for your feedback

Kelesys62 commented 6 months ago

Hello, changes done. You can check and merge if all ok else let me know. regards. Kelesys