Skip to content

Use Mbed config system to choose default flow control pins #99

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

Closed
wants to merge 1 commit into from
Closed

Use Mbed config system to choose default flow control pins #99

wants to merge 1 commit into from

Conversation

marcuschangarm
Copy link
Contributor

Old default made it hard to overwrite.

@@ -40,7 +40,7 @@ class ESP8266Interface : public NetworkStack, public WiFiInterface
* @param rx RX pin
* @param debug Enable debugging
*/
ESP8266Interface(PinName tx, PinName rx, bool debug=false, PinName rts=NC, PinName cts=NC);
ESP8266Interface(PinName tx, PinName rx, bool debug=false, PinName rts=MBED_CONF_ESP8266_RTS, PinName cts=MBED_CONF_ESP8266_CTS);

Choose a reason for hiding this comment

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

Does this still compile, if you are not using this board at all and those have not been defined?
That's the requirement this driver needs to pass.

@VeijoPesonen
Copy link
Contributor

I would like to reject this PR as I have a ticket to go through all these configuration settings. The goal there is that with Arduino form-factor defaults are ok but otherwise you're forced to define pins yourself.

@kjbracey
Copy link

kjbracey commented Oct 2, 2018

This particular change seems problematic because it's changing the "default" flow control pins for the case where you're manually specifying the data pins. If you're manually specifying the data pins, you surely wouldn't want to still be using the default flow control pins? If the default is (X,Y,Z,W), a manual specification of (A,B) should use (A,B), not (A,B,Z,W).

It would make sense to have the ability to set the default flow control pins alongside the default data pins for use in the default constructor. (Which you already can)

@kjbracey
Copy link

kjbracey commented Oct 2, 2018

Other configuration work is pending based on ARMmbed/mbed-os#8170

@marcuschangarm
Copy link
Contributor Author

The default hasn't changed, it's only using the default from the mbed_lib.json: https://github.com/ARMmbed/esp8266-driver/blob/master/mbed_lib.json#L12-L19

So, in the normal, default case you still get NC for both pins. If I'm using someone else's work and they are not using the flow control pins, I can still use the config system to enable flow control on my board.

@kjbracey
Copy link

kjbracey commented Oct 2, 2018

Not quite following. Don't you get the flow control by just changing the config, and using the default constructor?

Do you have an app manually specifying the RX/TX pins, and you're trying to change its behaviour?

@kjbracey
Copy link

kjbracey commented Oct 2, 2018

BTW, this seems very reminiscent of ARMmbed/mbed-os#6891 and ARMmbed/mbed-os#8046 where someone was suggesting something similar for Serial.

I think it's conceptually the same - if you had an ESP8266 on board with flow control, and the JSON was rigged up so that the default was that board, then you ran an app that manually specified ESP8266Driver(D0,D1) to get a second ESP8266 running, you would not want that manually-specified EPS8266Driver object to use the default flow control lines of the on-board one.

Okay, it's less likely that you have two ESP8266Drivers, but the point remains that if you're manually specifying the pins, that suggests you're trying to use a non-standard or second connection, which suggests you should not be taking any default pins from the JSON, but only the ones explicitly given in this constructor.

@marcuschangarm
Copy link
Contributor Author

You all make some excellent points! 😄

I'm closing this PR.

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.

4 participants