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

Support for other boards and shields #8

Merged
merged 1 commit into from
Apr 12, 2016

Conversation

sandeepmistry
Copy link
Contributor

This is a great library! With a few changes it could support running on non-ESP8266 boards as well.

In this patch, I've added an UDP instance argument in constructor to support more architectures. This way other networking libraries like Ethernet, WiFi, and WiFi101 can also be supported.

However, the tradeoffs are:

  1. This is a breaking change to the API
  2. Existing sketches that use this library require the following changes:
    • Add: #include <WiFiUdp.h>
    • Create WiFiUDP instance: WiFiUDP ntpUDP;
    • Pass WiFiUDP instance to constructor: NTPClient timeClient(ntpUDP, ....);

For Ethernet shield users:

  1. #include <Ethernet.h> is needed instead of #include <ESP8266WiFi.h>
  2. #include <EthernetUdp.h> is needed instead of WiFiUdp.h
  3. Create EthernetUDP instance instead of WiFiUdp.h

What do you think?

@FWeinb
Copy link
Collaborator

FWeinb commented Apr 8, 2016

This looks perfectly done. Thank you for that. I have no idea if it would be possible but It would be great to be able to detect various boards and do the right thing™ in choosing a default UDP lib. So the API could stay simple.

Any idea on how that could be achieved?

@sandeepmistry
Copy link
Contributor Author

This is very tricky, I don't have an answer unfortunately.

For non-ESP8266 boards, the user can plug in different shields (Ethernet, WiFi 101, etc.). Usually there's no built-in networking capabilities like the ESP8266's WiFi. As far as I know, there's no way to detect what shield is connected at run time. The user will also have to install the appropriate library to support the hardware via the library manager.

The new MKR1000 board has a built-in WINC1500 module that is supported by the WiFi101 library, so for this case a #ifdef ARDUINO_SAMD_MKR1000 could be used, in conjunction with an #ifdef ESP8266 for the ESP8266. However, unlike the ESP8266, the user has to also manually install the WiFi101 library.

I was discussing this earlier with @cmaglie, and the proposal in this PR was the optimal way to support the most boards as possible. It also future proofs things, because when a new Arduino compatible networking board or shield comes out, as long as it provides a class that extends the UDP class type, things will work out of the box in this library.

@FWeinb
Copy link
Collaborator

FWeinb commented Apr 8, 2016

👍 This sounds like a good reason for not doing some fancy tricks to get this working. Changing the API now sounds like the right move.

I don't have much time to maintain this library so I think it would be the best if I would give you commit access to this repository and you can add yourself as an maintainer, only if you are okay that?

@sandeepmistry
Copy link
Contributor Author

I don't have much time to maintain this library so I think it would be the best if I would give you commit access to this repository and you can add yourself as an maintainer, only if you are okay that?

Let me discuss this with @cmaglie next week and get back to you.

Thank you for reviewing and being open to this change.

@FWeinb
Copy link
Collaborator

FWeinb commented Apr 8, 2016

Ok. Will let this stay open till next week.

@sandeepmistry
Copy link
Contributor Author

@FWeinb it would be great if you could give myself and @cmaglie commit access to this repo.

We would also be open to transferring this library to the arduino-libraries Github organization.

@Testato
Copy link
Contributor

Testato commented Apr 11, 2016

+1 for transferring it to official arduino-libraries
FWeing has created a very important library for the Arduino environment

@FWeinb
Copy link
Collaborator

FWeinb commented Apr 11, 2016

@sandeepmistry I added you both as contributors to this library.

I am open for moving this into the arduino-libraries org too.

@sandeepmistry
Copy link
Contributor Author

@FWeinb great, thank you!

A few questions for you:

  • How do you see maintenance going forward?
  • Would you still like us to submit PR's for changes and review them?
  • Similarly, how does this apply to tagging new release?

@FWeinb
Copy link
Collaborator

FWeinb commented Apr 11, 2016

  • How do you see maintenance going forward?

I would like to bring this to 3.0.0 with your additions and than only do bug fixes.

  • Would you still like us to submit PR's for changes and review them?

As long as there is no API change involved you can merge it. When there are API changes I would like to review it.

  • Similarly, how does this apply to tagging new release?

As long as it are patches you can do a release.


In general I would like to move this to the arduino-libraries org.

@sandeepmistry
Copy link
Contributor Author

Ok, this sounds good.

Please go ahead with a 3.0.0 release if things look good. We might make some other changes in the next week or two, but can deal with those later.

As for transferring to arduino-libraries, whenever you are ready, please transfer to @cmaglie. Then he can move to arduino-libraries. We'll also have to update the URL used in the library manager accordingly, but we'll take care of this.

@FWeinb
Copy link
Collaborator

FWeinb commented Apr 11, 2016

Will I keep commit rights after transferring? If that is the case I would start by transferring to @cmaglie and than start merging and release 3.0.0

@cmaglie
Copy link

cmaglie commented Apr 12, 2016

The 'write' permission should be kept after the transfer IIRC.
By the way I'll keep an eye on that and re-add the permission, if needed.

@FWeinb
Copy link
Collaborator

FWeinb commented Apr 12, 2016

Okay. Just started the transfer.

@cmaglie
Copy link

cmaglie commented Apr 12, 2016

Done, it seems that all the permissions have been preserved, let me know if you find any problem.

@FWeinb
Copy link
Collaborator

FWeinb commented Apr 12, 2016

Everything look good! Let's make a plan for 3.0.0. I will merge this now and open an issue for what we want in 3.0.0

@FWeinb FWeinb merged commit b910b12 into arduino-libraries:master Apr 12, 2016
@FWeinb FWeinb mentioned this pull request Apr 12, 2016
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