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 Write command to the gattc implementaitons (windows hat already one) #205

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Fabianexe
Copy link
Contributor

This tries to solve issue #153 and add a Write function to all gattc implementations (windows hat already one).

I am on a Mac and the Darwin implementation works fine for me.
The linux implementation is taken form @aykevl comment.
For sd implementation I have looked in the api reference and it seems like the simple change form WRITE_CMD to WRITE_REQ does the trick.

However, I have no possibility to test the linux or sd implementation properly.
Thus, it would be welcome if somebody else could verify that they work es attended.

@aykevl
Copy link
Member

aykevl commented Dec 25, 2023

However, I have no possibility to test the linux or sd implementation properly.
Thus, it would be welcome if somebody else could verify that they work es attended.

The quickest way to get this change merged is to only send tested code. If you can't test on Linux or with the SoftDevice, I suggest leaving those implementations out.
(As a reviewer, it takes a lot of time to test every change so if you can confirm all the code you submit actually works it saves a lot of time - if you can confirm it works and the code looks reasonable, I'm fine with merging it).

@Fabianexe
Copy link
Contributor Author

I have put the from me testet Mac part into a new PR #212

I am not sure what to do with this PR.
After all it would be nice if write would be available on all systems.
Thus, if somebody test it further parts could me merged.

@deadprogram
Copy link
Member

@Fabianexe looks like the code needs to be go fmt.

Also, does the newly merged #216 change anything in your implementation?

@aykevl
Copy link
Member

aykevl commented Jan 4, 2024

Yes this PR will need to be updated after #216.

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.

3 participants