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

Replace to strncmp from strcmp on examples/bluetoot/gatt_client #502

Closed
wants to merge 1 commit into from
Closed

Replace to strncmp from strcmp on examples/bluetoot/gatt_client #502

wants to merge 1 commit into from

Conversation

asukiaaa
Copy link
Contributor

@asukiaaa asukiaaa commented Apr 8, 2017

Thanks for useful example projects.

I tried to use GATT client project but I cannot establish connection between server and the client.
I found that strcmp does not return expected result on my environment so I changed the value.

My environment

  • OS: Ubuntu16.10 64bit
  • GCC: version 6.2.0 20161005 (Ubuntu 6.2.0-5ubuntu12)

After this modification, GATT client proceed connection to found server device.

Thanks.

@CLAassistant
Copy link

CLAassistant commented Apr 8, 2017

CLA assistant check
All committers have signed the CLA.


if (adv_name != NULL) {
if (strcmp((char *)adv_name, device_name) == 0) {
if (strcmp(adv_name_chars, device_name) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Am I right to think that it would be sufficient to replace this strcmp with strncmp((char *)adv_name, device_name, adv_name_len)?

Allocating a 256 byte array on the stack may cause stack overflow issues...

@asukiaaa
Copy link
Contributor Author

@igrr
Thanks for your advice.
I succeeded in connecting device with using strncmp.

Is ts ok that I commited 2 times for this pull request?
If it's not suit for some rule of this repository, I will amend or create other pull request that correct one line for strcmp.

Thanks.

@igrr
Copy link
Member

igrr commented Apr 11, 2017

If you can squash two commits and force-push to this branch, that would be perfect.

@asukiaaa
Copy link
Contributor Author

Done, how about it?

@igrr
Copy link
Member

igrr commented Apr 11, 2017

Thanks, looks good. Copying this into the internal review and merge queue.

@asukiaaa
Copy link
Contributor Author

Thanks.

@asukiaaa asukiaaa changed the title Change value for strcmp on examples/bluetoot/gatt_client Replace to strncmp from strcmp on examples/bluetoot/gatt_client Apr 11, 2017
@igrr igrr added the Status: Pending blocked by some other factor label Apr 11, 2017
igrr pushed a commit that referenced this pull request Apr 25, 2017
…ooth/gatt_client

(Amended slightly from version in #502 to
account for differences when adv_name is a prefix of device_name.)
igrr pushed a commit that referenced this pull request Apr 25, 2017
Fix device_name check in gatt_client example

`strcmp` was used against `adv_name` array, which was not a zero terminated string, causing `strcmp` check to fail for valid names.

Ref. #502

See merge request !652
@projectgus
Copy link
Contributor

Cherry-picked in a797dca. This merge took a little longer than expected because we ended up tweaking the commit a bit (for the case where device name is a prefix of adv_name, or vice versa.) Thanks for identifying the problem and sending the fix. :)

@projectgus projectgus closed this Apr 25, 2017
@asukiaaa
Copy link
Contributor Author

asukiaaa commented Apr 26, 2017

@projectgus
Thank you for cherry picking.

@igrr igrr removed the Status: Pending blocked by some other factor label Nov 14, 2017
0xFEEDC0DE64 pushed a commit to 0xFEEDC0DE64/esp-idf that referenced this pull request May 5, 2021
* Add files via upload

Calls to writePattern() don't send the desired number of bytes when the pattern size doesn't divide evenly into the hardware FIFO size (e.g. sending 18-bit RGB data, 11 patterns take 63 bytes, so 1/64th of the data is never sent).

* Add files via upload

Remove white space changes.
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