Added Function To Check the Serial Connection From CircuitPython Layer

[sommersoft]

See isREPLconnected & isUSBconnected issue #544.

This only includes atmel-samd and nRF for now. ESP8266 will require more work since supervisor is not an entity on that port.

This is my first [sub]module from scratch, so please let me know if I totally bungled it. I kind of patchworked my way through it, after initially just putting it all in __init__ then realizing that wouldn't work across ports.

[dhalbert]

supervisor/Status.c line should have a single tab, not spaces.

[sommersoft]

argh! I meant to go back and fix that before pushing...

[tannewt]

@dhalbert did you have more feedback on this. I think we talked about it last week.

[dhalbert]

@sommersoft I built this and tried it, but the use did not match the documentation:

>>> import supervisor
>>> supervisor.
__name__        enable_autoreload
disable_autoreload              set_rgb_status_brightness
Status
>>> supervisor.Status.serial_connected
True
>>> supervisor.serial_connected
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'module' object has no attribute 'serial_connected'
>>> 

Just doing supervisor.serial_connected is fine. no need to have a visible Status class.

[sommersoft]

Holy moly! I totally forgot to update the documentation after I moved it from shared-bindings/__init__.c to Status.

With regards to putting this in Status, the decision was mostly based on possible USB enumeration & REPL detection (discussion in #544), and how the ports vary/will vary on how they handle any of that (Feather52 for instance, doesn't use USB CDC, and has additional REPL options vice SAMD). The fact that all the functions in shared-bindings/__init__.c are SAMD specific is what initially pushed me to separate these, but that could probably be overcome with #ifdefs.

If you'd rather this goes back into shared-bindings/__init__.c, that's easy to work my way back there.

EDIT:

Just doing supervisor.serial_connected is fine. no need to have a visible Status class.

I may have misunderstood this. Is there is a way to leave it in Status, but have it callable using supervisor.serial_connected? I'll have to either get some direction or spend some time perusing code (if it's done elsewhere, that I missed).

[dhalbert]

If you think it would make sense to have a status subcategory, take a look at how we did microcontroller.cpu.temperature, where cpu is a singleton instance of Processor. cf http://circuitpython.readthedocs.io/en/2.x/shared-bindings/microcontroller/Processor.html.

I was not in the loop on #544. @tannewt do you have a comment?

[sommersoft]

I can further mimic cpu, no problem and I concur (was my initial intent actually...missed the cpu qstr in the globals_table). Like I said, I patch-worked my way through this one, utilizing microcontroller & usb_hid as roadmaps for the most part.

I'll wait for any further comments from tannewt before I start pushing forward with changes.

[tannewt]

How about renaming the supervisor module to circuitpython and making a Supervisor class and supervisor singleton (just like cpu)?

[dhalbert]

How about renaming the supervisor module to circuitpython and making a Supervisor class and supervisor singleton (just like cpu)?

I wonder about permanently branding it that way, in case someone wants to fork. So would serial_connected be circuitpython.supervisor.serial_connected? How about the other supervisor operations? Is there anything directly under circuitpython?

Maybe put some of this under board instead? board.set_rgb_brightness(), board.serial_connected? That location kinda makes sense to me.

enable_autoreload and disable_autoreload do make sense under supervisor or circuitpython. Actually, making it a .autoreload True/False property makes more sense.

[tannewt]

I don't know of anything we'd have directly under circuitpython yet. autoreload is two functions now because modules cannot have attributes that are backed by C.

I'm ok with it being called something else.

[sommersoft]

So I've taken some time to understand both of your points, and do some ponderin' and researchin'.

• circuitpython.xxx: I agree with Dan on the branding piece. However, CircuitPython already has a number of deviations from MicroPython with respect to uncommon ports (and even within common chipsets [esp8266]). I don't think anyone wanting to fork and deviate would have much more issue than you all have had with that deviation. circuitpython would also prove a natural spot for future things that neither fit into the current modules, CPython modules, nor have an obvious name. Kind of like serial_connected. 😄

  • What about using misc.xxx instead? I know it's an invitation to "throw it there" for all manner of things, but it has the flexibility of circuitpython without the branding.

• board.xxx:

  • I can see moving the set_rgb_brightness function here since the LEDs are on the board, and have pins associated. I can also see leaving it in supervisor since it's used to adjust the brightness on system load (at least I think it is...can't seem to find _boot.py atm). I previously mentioned "SAMD specific" because the current nRF boards don't have them; that may change?

  • I think serial_connected (and usb_enumerated if it becomes a thing) could go here. But, this will break from board = pins concept; which may be just my perception of the concept.

• supervisor.xxx: of the current options, this makes the most sense to me for serial_connected (and usb_enumerated & repl_active if they become a thing). supervisor is already kind of the central location to check on usb/serial state internally...it just isn't brought out to the user's level. USB and REPL also seem to fit into this construct to me as well.

As with most things I've done so far, I'm of the position that if you want this in a certain place and to function a certain way...that's what I'll do (or attempt).

[dhalbert]

Naming problems are hard. I forgot that modules can't have properties, which is one reason why we have microcontroller.cpu.temperature instead of microcontroller.temperature.

I'm guessing that @tannewt's idea of supervisor is kind of like os or sys. Some alternatives might be circuitpython, runtime, or system (which might be confused with sys). Since we don't have a real OS or even a tiny OS right now, it can be a little confusing.

I'm ok with supervisor.status.serial_connected, perhaps substituting some other name for supervisor. I could also see adding one or more other singleton objects, so for instance supervisor.runtime.rgb_brightness, supervisor.runtime.auto_reload, etc.

[siddacious]

The only contribution that I can think of is that misc, like utility is a bit of an anti-pattern. By that I mean that it is a tempting place to shove code that doesn't have a clear home and as a result can become a endless pile of code that has cleverly avoided being thought about for long enough to determine how it fits into the rest of the project.

That said it's clear that you folks are thinking about where this code should go so perhaps this comment doesn't apply to this scenario, but I thought I would just mention it.

[tannewt]

I like supervisor.runtime.

@sommersoft Newer nRF52 boards will likely follow the same "express" design of spare spi flash and status pixel.

[sommersoft]

I've come to the conclusion that serial_connected will not be possible on the ESP8266. Full details posted in #544.

Pending any changes after review or further discussion, this is ready to merge.

[dhalbert]

I like the naming now. I tested this with a CPX and the following main.py test program:

import board,neopixel, supervisor
pixels = neopixel.NeoPixel(board.NEOPIXEL, 10)
RED=(100,0,0)
BLUE=(0,0,100)
while True:
    pixels.fill(BLUE if supervisor.runtime.serial_connected else RED)

I am using a LiPo battery and the USB plug. If USB is connected, then the NeoPixels turn blue. They're red if USB is unconnected when main.py starts. However, if I then unplug USB, the pixels don't turn red, so .serial_connected is still True. I'm not sure if the state can be checked more dynamically or not.

[sommersoft]

On atmel, it calls usb_connected which calls cdc_enabled which calls cdcdf_acm_is_enabled. It appears that it should be returning a dynamic value. I can dig into that while working the REPL limit...they're in the same files.

[tannewt]

I don't think we handle connect, disconnect and then reconnect well. (ASF4 USB code doesn't reset correctly as far as I know.) This is an artifact of it.

[sommersoft]

I don't think we handle connect, disconnect and then reconnect well. (ASF4 USB code doesn't reset correctly as far as I know.) This is an artifact of it.

I haven't dug all the way back to the ASF level on this yet. But, in cdc_enabled, the first check is if variable mp_cdc_enabled == true and returns true. Only if mp_cdc_enabled == false does it actually call back to the ASF level to test the connection (cdc_acm_is_enabled). Furthermore, during that check, it doesn't set mp_cdc_enabled = false. So, need to find out if anything else sets the variable state...

[sommersoft]

NOTE TO SELF: revisit the nRF since the new version turns off the UARTtoUSB chip.

GOING FURTHER: will this drive a much wider update for all things USB?

[sommersoft]

RE: "Dynamic" USB Enabled Check
Here is the current usb.c/cdc_enabled:

static bool cdc_enabled(void) {
    if (mp_cdc_enabled) {
        return true;
    }
    if (!cdcdf_acm_is_enabled()) {
        return false;
    }
    cdcdf_acm_register_callback(CDCDF_ACM_CB_READ, (FUNC_PTR)read_complete);
    cdcdf_acm_register_callback(CDCDF_ACM_CB_WRITE, (FUNC_PTR)write_complete);
    cdcdf_acm_register_callback(CDCDF_ACM_CB_STATE_C, (FUNC_PTR)usb_device_cb_state_c);
    cdcdf_acm_register_callback(CDCDF_ACM_CB_LINE_CODING_C, (FUNC_PTR)usb_device_cb_line_coding_c);
    mp_cdc_enabled = true;

    return true;
}

I'm thinking this simple change can allow it to be a little more accurate when it's called:

static bool cdc_enabled(void) {
    if (!cdcdf_acm_is_enabled()) {
        mp_cdc_enabled = false;
        return false;
    }
    if (!mp_cdc_enabled) {
        cdcdf_acm_register_callback(CDCDF_ACM_CB_READ, (FUNC_PTR)read_complete);
        cdcdf_acm_register_callback(CDCDF_ACM_CB_WRITE, (FUNC_PTR)write_complete);
        cdcdf_acm_register_callback(CDCDF_ACM_CB_STATE_C, (FUNC_PTR)usb_device_cb_state_c);
        cdcdf_acm_register_callback(CDCDF_ACM_CB_LINE_CODING_C, (FUNC_PTR usb_device_cb_line_coding_c);
        mp_cdc_enabled = true;
    }

    return true;
}

[tannewt]

@sommersoft that looks ok to me. Did you change the files?

[sommersoft]

@tannewt @dhalbert, cdc_enabled is now updated to match the above. I tested on a Trinket with USB->PC and USB->Wall Wart; still functions as before. But, I don't have a battery yet (that I can easily hook up) to reproduce Dan's earlier test. After that test is run again, and if nothing else is needed, this should be good to merge.

I'll look into the new Feather52 USB stuff once I get one in hand.

[dhalbert]

@sommersoft did you get a battery yet to test removing the USB plug while still being powered up?

Also, could you merge from upstream now to fix the merge conflicts? Thanks.

[sommersoft]

So, @tannewt was correct that the problem is probably deeper in ASF. The above "simple" fix does not work after testing with a battery. Still stays "connected" after removing the USB connection.

Guess it's into the breach once more...

[sommersoft]

cdcdf_acm_is_enabled() returns a struct entry, which is set by respective cdcdf_acm_enable and cdcdf_acm_disable functions. In short...not dynamic. I'll keep pulling on these tomorrow to see where they're implemented.

Also, using the device class (usbdc) as opposed/in addition to the cdc class could be another option. cdcdf_acm_init uses a usbdc function, which returns one of these states. Could maybe just nest that into cdc_enabled by ORing it into the first conditional: if (usbdc_get_state < USBD_S_DEFAULT || !cdcdf_acm_is_enabled()).

[tannewt]

I don't think we should block this PR on fixing the after USB case. Its useful enough on start up. We likely won't do multiple USB enumerations until we switch to TinyUSB from ASF4.

[dhalbert]

@sommersoft I agree with @tannewt that this is useful as is. Could you just add some documentation caveat that says this will detect if USB was connected at the time the board started, but doesn't detect disconnect (also connect? -- I don't know the answer) later? Then we'll merge it in.

[sommersoft]

@dhalbert it does catch the connect scenario (just tested BATT only -> BATT+USB). Just disconnect isn't caught. Working on some wording now.

[tannewt]

Awesome job on this @sommersoft. Thank you so much!