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

Implement socket_bind and socket_accept #41

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

Conversation

janjongboom
Copy link
Contributor

@janjongboom janjongboom commented Jul 28, 2017

@geky @sarahmarshy

Work in progress... Tested with https://developer.mbed.org/teams/ST/code/mbed-os-tcp-server-example/

I'd like some feedback on the approach (working around the ATParser library quirks) before cleaning it up.

@janjongboom
Copy link
Contributor Author

What I basically need is a function that I can call every X ms. which says: "Do you have any data in your buffers that has CONNECT or CLOSE in them". Then, as long as the RxIrq is attached properly we shouldn't need this weird pinging (it's also not thread-safe).

@janjongboom
Copy link
Contributor Author

A problem with the ATParser library is that it only processes information when you ask it to, which is great for simple request/response patterns, but not for real OOB data like sockets connecting. I implemented a second buffer which handles CONNECT and CLOSED commands. This way we cannot miss any commands. Also, added some logic for sockets disconnecting, which currently causes the library to hang indefinitely on the recv() call. This PR also needs ARMmbed/ATParser#10.

@janjongboom
Copy link
Contributor Author

Ping @geky @sarahmarshy

@janjongboom
Copy link
Contributor Author

Ping again @geky @sarahmarshy

@SeppoTakalo
Copy link
Contributor

Outdated, closing...

@janjongboom janjongboom reopened this Dec 6, 2017
@janjongboom
Copy link
Contributor Author

@SeppoTakalo Since when is outdated a reason to close issues without discussion?!

I want to have a discussion about the approach first before rebasing this.

/cc (again!) @sarahmarshy @geky

@SeppoTakalo
Copy link
Contributor

For issues, its not valid but for pull requests it is a valid reason.

Ownership of this driver is now transferred to my team and I only checked that this PR seems to be abandoned for a while, therefore I closed.

ESP driver have gone lot of rework since this have been published so I cannot know whether this is valid anymore. For improvements, I would like to first see the failing testcase or liked issue.

From testcases, I can see that this driver fails on accept() and bind() tests so this PR might be valid, but it needs to be updated before we can review it.

@janjongboom
Copy link
Contributor Author

@SeppoTakalo Yes, it's abandoned because no-one has given a second to look at the PR. What am I supposed to do in that case? I've been trying to get someone to look at the way this is implemented for the last 5 months.

accept() and bind() are not implemented in the ESP8266 driver, and this is one way of implementing it. It would be good if someone could evaluate the way that I implemented this, and see if it's a good approach. If so, it can be rebased on master.

@geky
Copy link
Contributor

geky commented Dec 7, 2017

The code shouldn't have divereged too much. Not just this pr, but all features going into ESP8266Interface have been frozen for a while (sorry @janjongboom!).

@SeppoTakalo
Copy link
Contributor

This is now ticket ONME-3341 and we can take ownership of the work within our sprints.

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

3 participants