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

wipy: Consider moving "operation mode" param from network.WLAN() constructor to a method #1319

Open
pfalcon opened this issue Jun 11, 2015 · 35 comments

Comments

@pfalcon
Copy link
Contributor

pfalcon commented Jun 11, 2015

We should set meta-interface for "network" module. Proposed points:

  1. Constructor of classes in network module should accept only arguments related to hardware parameters. Good example is http://docs.micropython.org/en/latest/library/network.html#network.CC3K - a cc3k module can be attached to any SPI port, so it takes that as an argument. If hardware in question is "builtin" and comes in single instance, there should be no arguments to constructor. If there're number of similar builtin hardware blocks, there should be single param, and identifier of particular block.
  2. For builtin WiFi hardware, class name should be WLAN (really? were alternatives like WIFI, WiFi considered?)
  3. All other param should be configurable via other methods, and it should be possible to reconfigure them on the fly. E.g., as station/ap mode for WiFi. If on the fly reconfig is not possible, a particular port should describe that limitation.
@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 11, 2015

Caveats and concerns with the proposed implementation:

There's a dichotomy between "networking hardware" and "networking interface". It's fine distinction on case-by-case basis what a "network" class represents. For example, an ethernet router with 5 VLAN ports would likely have an object per port.

A WiFi case is interesting, because oftentimes, there's really 2 interfaces, one working in STA, another in AP mode, which can work together or each alone. But representing a WiFi module like that would probably be only confusing to the users, so I'd propose to treat it as a a singled bridged interface and thus single "network" class. Or maybe not.

@danicampora : can you confirm whether it's possible in wipy to instantiate both WLAN(WLAN.STA) and WLAN(WLAN.AP) at the same time?

@pfalcon pfalcon changed the title wipy: Please move "operation mode" param from network.WLAN() constructor to a method wipy: Consider moving "operation mode" param from network.WLAN() constructor to a method Jun 11, 2015
@danicampora
Copy link
Member

Constructor of classes in network module should accept only arguments related to hardware parameters. Good example is http://docs.micropython.org/en/latest/library/network.html#network.CC3K - a cc3k module can be attached to any SPI port, so it takes that as an argument. If hardware in question is "builtin" and comes in single instance, there should be no arguments to constructor. If there're number of similar builtin hardware blocks, there should be single param, and identifier of particular block.

Why? As I see it, it should follow the same model as other peripherals like UART, SPI, I2C, etc. When you construct the object you configure the peripheral at the same time (specify mode, baudrate, etc.). Setting the operating mode of the CC3200 is an expensive operation (in terms of time). Configuring the operating mode as STA is simple, but as an AP you must specify the ssid, security, key, channel. At least in the case of the CC3200 changing any of these params involves turning OFF and the ON the network processor. It's really hard for me to see a scenario where you want to change the operation mode many times on the fly, the same way as you don't do it with a SPI or I2C bus.

Is it possible to change such params on the fly with the ESP8266?

I try to avoid implementing several methods to configure single params in order to save space, specially when I don't see the need of having such "flexibility".

For builtin WiFi hardware, class name should be WLAN (really? were alternatives like WIFI, WiFi considered?)

It was an arbitrary choice, I don't mind changing it to WiFi. WLAN is the name used on Windows, (and I believe also on Linux), to refer to the wireless network card.

@danicampora : can you confirm whether it's possible in wipy to instantiate both WLAN(WLAN.STA) and WLAN(WLAN.AP) at the same time?

No, it's not possible, it's either one or the other.

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 11, 2015

Is it possible to change such params on the fly with the ESP8266?

Yes, sure. That's how it started - with #1318 adding a method to do that. The same is possible with Linux wireless interfaces too - you say "iwconfig mode " and it switches.

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 11, 2015

both WLAN(WLAN.STA) and WLAN(WLAN.AP) at the same time?

No, it's not possible, it's either one or the other.

Ok, so the fully technically correct solution is to allow to instantiate 2 interfaces - because there're 2 interfaces, and most of params can be configured independently. The issue, such configuration would be cumbersome for most users and most usecase, so we should target a simplified initially. But then there should be a separate method to set a mode, because it's generalized model of how WiFi module works.

@danicampora
Copy link
Member

Yes, sure. That's how it started - with #1318 adding a method to do that. The same is possible with Linux wireless interfaces too - you say "iwconfig mode " and it switches.

OK, what about making the constructor empty (for esp8266 and wipy) and add a config() method to query the current configuration and also to set it. It could follow this signature:

nic.config(mode, ssid, *, security=WLAN.OPEN, key=None, channel=5)

if mode=WLAN.STA, the rest of the params are not needed.

?

@danicampora
Copy link
Member

We should also have an independent method to set the output power. Maybe the channel could also be set via an independent method.

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 11, 2015

What about making all args keyword then, so they can be updated on each own? ;-)

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 11, 2015

We should also have an independent method to set the output power. Maybe the channel could also be set via an independent method.

Yep, most of params can be set independently ;-). I don't know how to balance number of methods, but having whole bunch of kwargs and single .config() method may be not that bad a compromise. Need more thinking and more people providing feedback.

@danicampora
Copy link
Member

Yep, most of params can be set independently ;-). I don't know how to balance number of methods, but having whole bunch of kwargs and single .config() method may be not that bad a compromise. Need more thinking and more people providing feedback.

I also think that making all params kwargs is a good option. Let's give it some thought and wait for more feedback like you said.

@danicampora
Copy link
Member

Maybe we can have a big powerful config() method that accepts a bunch of kwargs, and also other "optional" methods to set/get params independently for the ports that can afford it.

@danicampora
Copy link
Member

What I don't like about the config name is that we already have an ifconfig method to configure the IP address, and it might be a bit confusing to have config and ifconfig at the same time.

Edit: we could have ifconfig and iwconfig similar to linux then...

@owens-bill
Copy link
Contributor

Is there any way to have this config/iwconfig method return current values? Maybe if it has no arguments it does a bunch of get calls and returns a dict of argument names and values? That's a bit more expensive and harder to deal with if you only want one thing, though.

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 12, 2015

@owens-bill : Very good point, which we missed in the previous discussion. So, one function with kwargs isn't really panacea, it then should be backed by a "get" function accepting what? A keyword as string to query a particular param's value? I personally find using strings as enumeration values ugly, and here we will even get asymmetry between set and get functions...

@danicampora
Copy link
Member

Maybe if it has no arguments it does a bunch of get calls and returns a dict of argument names and values?

The idea (I think) is to return a namedtuple, so you can do:

wlan.iwconfig().mode, wlan.iwconfig().channel, and so on.

Maybe it's a bit more expensive. But I don't think it's hard to deal with.

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 12, 2015

In the meantime, the best thing I can do is to introduce "network" module for esp8266: ee3fec3 and start to move suitable functions there: 32eb4b9

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 12, 2015

@deshipu : You know what that ^^^ means: prematurely written documentation now needs to be rewritten ;-).

@deshipu
Copy link
Contributor

deshipu commented Jun 12, 2015

@pfalcon where I come from, the documentation is written when you start coding, and updated in the same commits that change how the code works. It's really not much more work than writing a commit message. Of course, I can fix the documentation later, but the problem is, I have no idea what to change and how, because you are the person making the changes, so you are best qualified to describe them.

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 12, 2015

the documentation is written when you start coding

NP, feel free to write both documentation and code ;-)

but the problem is, I have no idea what to change and how,

And that's why we have tickets like this, so all interested people could find a sustainable solution and know about it. And as a full disclosure, esp8266 is experimental port and will be broken port while it's based on the broken vendor SDK. Which means there will always be someone wanting to "fix" it (well, again while esp8266 hw will pose interest and won't be displaced by something else, then the story of ever-brokenness will repeat). By that logic, the best place to apply effort is getting rid of broken vendor SDK. Feel free to contribute there too.

@deshipu
Copy link
Contributor

deshipu commented Jun 12, 2015

@pfalcon I'm very sorry for writing that documentation. Feel free to remove it entirely if it's getting in your way and stopping real development from happening.

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 12, 2015

@deshipu : No, I mean please let everyone else know if you're ready to maintain it. If not, and nobody else would step in to maintain it, then sad choice may be indeed to remove it to not give users incorrect information. I for one can't maintain it (I won't be referring to what I'm doing where I come from, but you can see what I'm doing merely here on github - https://github.com/pfalcon . Btw, if there's such page on bitbucket which lets overview user activity over extended period of time, let me know.)

@deshipu
Copy link
Contributor

deshipu commented Jun 13, 2015

Thanks, but no, thanks. I'm not going to be your personal documentation writer, @pfalcon. If you can't be bothered to document your own changes, then your code will forever remain undocumented and unusable, because only you know and care how it works.

Don't get me wrong. I wish this project very well. I admire the work you all are doing on it. I try to help when I have the time and find a task suitable for my skill (which is quite diminutive when C is involved). However, I am not your employee and you do not get to dictate where I am to apply my effort, son. And no, a dick-comparing contest over who has more commits over on Github will not change that.

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 13, 2015

@deshipu : You can't be my personal documentation writer, this is community project. The changes I applied are not "mine", but bring esp8266 in consistency with interface used by other ports - http://docs.micropython.org/en/latest/library/network.html#network.CC3K link was given at the description of this ticket, another one: http://micropython.org/resources/docs/en/latest/wipy/library/network.html . I in particular proceeded with making those changes because 1) nobody else is doing them over all this time; 2) there were >1 submission trying do to those things in adhoc way to esp8266, so not having good guidelines for people to follow will lead to them wasting more effort.

However, I am not your employee and you do not get to dictate where I am to apply my effort

I don't know how you get that idea in the first place. Please re-read the original message:

@deshipu : You know what that ^^^ means: prematurely written documentation now needs to be rewritten ;-).

It addresses you, as you're the author of the original submission and would be natural party to discuss its future. But nowhere it says that you should update it. Vice-versa, it carefully uses passive voice to avoid pitfall of implying it's somebody's specific task. The above phrase didn't have any other intention but to ping you and see if you'd be interested in maintaining it (I perfectly know that most of submissions are once-off and people "disappear"). Obvious replies would be "", "I don't have time", "I won't maintain it". But you decided to go personal, and so it went.

So, personal stuff should stop here. But seeing you like to stir quarrels on empty grounds, makes me worry - do you understand OpenSource licensing well, and the fact that you submit contribution to MicroPython on the terms of MIT license? As a quick check, do you agree with following statement: "Once you release code as open-source, it no longer belongs to you, but to community. As an author, you still have responsibility about it, but it's community decides in the end what happens with the code." If you don't agree, I'd prefer your patch removed purely because of possible licensing issues.

Thanks.

@danicampora
Copy link
Member

Going back to the original discussion...

In a not so distant future, we will add support for P2Pmode (WiFi direct), and that will probably require having some other configuration options. This makes me think that having only keyword arguments for iwconfig(...)makes the most sense. The same goes when calling iwconfig()without params to query the current configuration, a namedtuple is easier to read and to use.

@deshipu
Copy link
Contributor

deshipu commented Jun 13, 2015

I don't know how you get that idea in the first place. Please re-read the original message:

I wasn't replying to the original message, but to the subsequent messages, in which you tell me to work on other parts of the project and ask me to commit myself to keeping the documentation up to date. I do know how open source projects work, that's why it was such a shock for me to see such "all or nothing" ultimatum from you.

Going back to the original message, there is really nobody else who can document your changes for you. Going through all the merged commits, figuring out what exactly changed and what the author had in mind, and reverse-engineering it all to guess what needs to be changed in the documentation is a very inefficient way of doing it, and I think you will have a hard time finding someone willing to do it for you. For instance, if I had enough knowledge of C and Python API to figure it all out, I would rather be working on those other things you wanted me to work on.

Making changes to the documentation by the same person who makes them in code, in the same commit, makes sure that the documentation is up to date and accurately reflects the intention of the author of the changes. If you can't do that much, then maybe you shouldn't be doing the changes in the first place.

Sure, there is also the reviewing of the documentation changes, correcting phrasing and markup, expanding the examples, etc., and I do plan on doing that.

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 13, 2015

I wasn't replying to the original message, but to the subsequent messages

Subsequent messages were reply to your message where you "taught" (note quotes and take it easy) that on your paid job you it differently (paid job implied, I "invited" you to elaborate on your open-source activity (your bitbucket account is probably the most active I ever saw), but you "skipped" that either). It's obvious that on paid job people do it differently, to not point out this obvious fact I invited you to do it differently than we here in FOSS project.

ultimatum

Come on, you can't not know what smiles mean, and that "ultimatums" don't come with smiles.

a hard time finding someone willing to do it for you

I was surprised you submitted that patch. I wanted to say that the port is in flux right then, but decided not discourage contribution like that (indeed, hardly anyone else would go for it).

Making changes to the documentation by the same person who makes them in code, in the same commit, makes sure that the documentation is up to date and accurately reflects the intention of the author of the changes.

Now we just need to back that with someone giving enough time to work on that, which means someone giving a paycheck. If you know someone interested, let us know.

If you can't do that much, then maybe you shouldn't be doing the changes in the first place.

That's not how open-source projects work - most of them come without documentation at all, many have docs appearing much later in their lifecycle, contributed by "users". Few have enough backing to actually write docs as part of the project. Pyboard and WiPy ports are such projects, so any other port should take them (and their docs) as affinity.

Sure, there is also the reviewing of the documentation changes, correcting phrasing and markup, expanding the examples, etc., and I do plan on doing that.

Correcting markup - moving bunch of functions from one module to another - is exactly what's needed here.

So, whatever details we discuss, it all comes down to a question from a community interested in sp8266 port (not from me personally) whether you're interested in taking maintainership of these docs or not (that's courtesy question after all - you started it and so far you the only one to contribute).

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 13, 2015

The same goes when calling iwconfig() without params to query the current configuration, a namedtuple is easier to read and to use.

So, iwconfig() with at least one param means set, no params - get with named tupled returned, correct?

Generally, I agree, just need to arrive right there with esp8266 port (while resolving side issues like above), then I'll be able to provide more feedback. But I'm rather sure that constructor should take only hw-related config, so if you don't have a strong reason to disagree, I'd appreciate if you could go with that change for wipy (per your priorities of course).

@deshipu
Copy link
Contributor

deshipu commented Jun 13, 2015

@pfalcon To cut the discussion, I am interested in working on making the documentation better and more useful, and I plan on devoting some of my time, irregularly, to improving parts of it. I'm not interested in trawling the commits in search of documentation-breaking changes and updating the documentation based on the information guessed from the diffs, because the author was too lazy to update the documentation along the way. I'm still not sure which of the two you mean by "maintaining", but I hope this clarifies it for you.

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 13, 2015

@danicampora : I'll let you know when enough functions will be moved from "esp" module to "network" so it will make sense to update docs in big chunk. If you'll be doing that, you will find "network" docs for other port useful, so hopefully will note that this ticket has links to them for reference.

@danicampora
Copy link
Member

So, iwconfig() with at least one param means set, no params - get with named tupled returned, correct?

Yes.

But I'm rather sure that constructor should take only hw-related config, so if you don't have a strong reason to disagree, I'd appreciate if you could go with that change for wipy (per your priorities of course).

Of course I agree, if we bring iwconfig() in then it's OK to make the constructor hardware related only. In the case of the esp8266 and the cc3200 such constructor will receive no params. I'll make those changes to the cc3200 and update the docs as well ;-).

@owens-bill
Copy link
Contributor

@danicampora Regarding the ability to set the channel on the ESP8266, it appears that it can be set for "sniffer mode" and when enabling the Soft AP, but cannot be set for regular station operation. The radio will be set to whatever channel the chosen AP is operating on when wifi_station_connect() is called. Moreover, if the chip is set for AP mode and station mode is enabled (either by switching modes or by starting in station+AP) then the AP channel will be forced to change, since there's only one radio: http://bbs.espressif.com/viewtopic.php?f=10&t=324 There's also no ability to set radio power, as far as I can tell.

@roger-
Copy link

roger- commented Jun 14, 2015

Is there a list of parameters that're candidates for iwconfig()?

Doesn't seem like the best idea to retrieve every wireless setting when you just want the RSSI or something.

@danicampora
Copy link
Member

Well, the RSSI is not a configurable param, so won't be part of iwconfig.

On Jun 14, 2015, at 4:25 PM, Roger notifications@github.com wrote:

Is there a list of parameters that're candidates for iwconfig()?

Doesn't seem like the best idea to retrieve every wireless setting when you just want the RSSI or something.


Reply to this email directly or view it on GitHub.

@owens-bill
Copy link
Contributor

Is there a list of parameters that're candidates for iwconfig()?

Here's what I was able to scrape out of the SDK docs:

  • opmode - station/soft-AP/station+AP
  • opmode default (saved in flash)
  • ap_number - count of AP's that will be remembered (up to 5)
  • ap_change - which remembered AP to connect to (but this conflicts with WLAN.connect, in some ways)
  • auto_connect - whether to connect to a saved AP when powered on
  • dhcp mode - I think this belongs with ifconfig since it's IP-related
  • reconnect - whether or not to reconnect to the AP - don't know why someone would want to disable this but it's there
  • phymode - B/G/N
  • mac
  • sleep type - none/light/modem

There's a bunch of soft-AP config, but that seems like it should all be in a separate method since it isn't related to the 'interface' per se.

I'm not sure what the use cases are for managing the AP list, and I don't feel like those functions play well with the iwconfig() model since they cross over into what WLAN.connect() does. If they're implemented I'd argue for a separate method to set and get them. I'm also not convinced of the value of setting the default operating and connection modes, since I think we're assuming that a MicroPython program will be running every time the chip powers up, right? But it isn't hard to add them.

So the params could look like this, using the SDK constants since some of them haven't been defined in MicroPython yet:

WLAN.iwconfig(opmode = STATION_MODE, opmode_default = STATIONAP_MODE, phymode = PHY_MODE_11N, sleepmode = NONE_SLEEP_T, mac = b'\x18\xfe4\x9d\xc8\x85', reconnect = True)

That method would subsume esp.phy_mode, esp.sleep_type, esp.mac and my proposed esp.opmode, and add the ability to set default opmode and the reconnect flag.

@owens-bill
Copy link
Contributor

I wrote:

There's also no ability to set radio power, as far as I can tell.

In fact, there is; I was looking at outdated SDK documentation, and the function was added in 1.1.0 (which is what we're inheriting from esp-open-sdk as of right now). system_phy_set_max_tpw() allows setting the maximum transmit power in 0.25 dBm steps from 0 to 20.5 dBm, though most sources report that the maximum power is actually 19.5 dBm, with some only claiming 18.5.

@pfalcon
Copy link
Contributor Author

pfalcon commented Jul 25, 2015

Further discussions happens in the original issue on this matter: #876

tannewt added a commit to tannewt/circuitpython that referenced this issue Nov 8, 2018
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

No branches or pull requests

5 participants