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

I2c master slave example #5464

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

Conversation

suculent
Copy link
Contributor

@suculent suculent commented Dec 9, 2018

Fresh merge against current master. May fail with type changes to i2c core.


This change is Reviewable

@devyte
Copy link
Collaborator

devyte commented Dec 12, 2018

Fixes #5288

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 22, 2018

Would you like help to remove bearssl + lwip2 changes from this PR ?

@suculent
Copy link
Contributor Author

suculent commented Dec 22, 2018 via email

@devyte
Copy link
Collaborator

devyte commented Dec 22, 2018

@suculent your PR has not been merged yet. The lwip changes are still included, and are now conflicting. What needs to happen is one the following:

  • you need to revert the lwip stuff in your branch and push the update
  • you need to redo the PR and make sure only the examples are inccluded, and not lwip changes
  • @d-a-v or I can pull this PR, revert the lwip stuff locally, and make a new PR

@d-a-v was referring to the third point 😃

@suculent
Copy link
Contributor Author

suculent commented Dec 22, 2018 via email

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 22, 2018

I do understand it’s urgent.

Not that urgent

I’ve created it from scratch and it’s been already merged.

This one is not merged yet. You have to be careful of what is committed with git.
If you use command line, do not

git commit -a

prefer

git add -p
git commit

If you use a graphical interface, then next time maybe unselect changes that are not yours (everything you did not modify) before committing.
Either way, I think it is easier to make a new pull-request.

edit
The command line way would be

cd root-of-the-git-repository
git checkout i2c_master_slave_x
git diff master > patch
git checkout master
git pull
git checkout -b i2c_master_slave_y
patch -p1 < patch
git add -p
git commit
git push suculent/i2c_master_slave_y

(suculent/i2c_master_slave_y or whatever/i2c_master_slave_y, your local name of your personal distant fork)

@devyte
Copy link
Collaborator

devyte commented Feb 4, 2019

I tried the two examples here, and couldn't get them to work even after playing with the code for a while.
Notes:

  • start sequence should be first get the slave up, then the master, otherwise the sequence handling errors out (and doesn't recover)
  • comment about the pin numbers seem to have the numbering reversed (wemos d1 mini r2)
  • the msg struct has size 32 bytes, but only 30 bytes of members, which means there are two bytes of padding. This is obvious from lookong at the struct ember ordering.
  • The xmit includes those padding bytes, which are garbage, because the structs are never zeroed out.
  • When receiving the msg, bytes are copied into a char array of size 32, and then a null term is assigned at the end of the array. I think the struct is then copied over, and if the last padding byte is garbage, the null term could get lost. Printing then happens with garbage at the end.
  • Retransmit request doesn't seem to work.

@devyte devyte modified the milestones: 2.5.0, 2.6.0 Feb 4, 2019
@devyte
Copy link
Collaborator

devyte commented Feb 4, 2019

Pushing back, as this needs more work.

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 26, 2019

@suculent Do you plan to fix your PR ?

I also wonder if void (*TwoWire::user_onReceive)(int => size_t); is a breaking change.

@d-a-v d-a-v added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label Sep 26, 2019
@devyte
Copy link
Collaborator

devyte commented Oct 29, 2019

Pushing back again.

@devyte devyte modified the milestones: 2.6.0, 2.7.0 Oct 29, 2019
@suculent
Copy link
Contributor Author

suculent commented Oct 29, 2019 via email

@devyte
Copy link
Collaborator

devyte commented Oct 29, 2019

CC @Tech-TX

@devyte devyte removed this from the 2.7.0 milestone Dec 18, 2019
@devyte
Copy link
Collaborator

devyte commented Dec 18, 2019

Reducing 2.7.0 scope. This is to be handled as part of the ongoing Wire rework.

@earlephilhower earlephilhower added the merge-conflict PR has a merge conflict that needs manual correction label Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-conflict PR has a merge conflict that needs manual correction waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants