Improve plugin detection in busprobe()

[tmk]

I like to share this busprobe() patch and need code review and your thought on it.

Seems like MAX3421e CONDETIRQ(Connect/Disconnect Interrupt) doesn't arise for every plugin event and the chip fails to detect it sometimes. (Meanwhile, I never seen the chip failed in detection of unpluging.)

For confirmaion I tried to catch INT pin change and read bus state immedately using ISR(), but it still failed to detect device insertion event occasionally. So I believe that we cannot rely solely on the CONDETIRQ interrupt for plugin detection.

This patch checks bus state at every Task() call when vbusState is SE0(no device) regardless of INT pin.
There are some descriptions of the fix here also. tmk#9

These issues are related probably. #425 #452

[Lauszus]

Shouldn't you wait for the operation to finish like how it is done elsewhere in the code?

I.e:

while(!(regRd(rHCTL) & bmSAMPLEBUS)); //wait for sample operation to finish

[tmk]

That code seems to use contrary logic according to MAX3421E reference?
From my tests the waiting code does nothing and is not needed in fact.

The chip reference(AN3785.pdf p.52) says:

The CPU sets the SAMPLEBUS bit to instruct the SIE to sample the state of the D+ and D-
lines. The SIE clears the SAMPLEBUS bit when it completes the operation.

But the SAMPLEBUS bit shows strange behaviour there in fact.

I never seen 'S' with code below.

while (!(regRd(rHCTL) & bmSAMPLEBUS)) { Serial.print("S "); };

While this code is repeated forever for some reason.

while (regRd(rHCTL) & bmSAMPLEBUS) { Serial.print("S "); };

[Lauszus]

I guess the SAMPLEBUS bit is cleared in between the two calls.

[tmk]

With some more tests the SAMPLEBUS bits doesn't seems to be cleared until bus state is changed but not always. This is clearly not what the reference describes, we can't use the bit to wait for sampling bus state anyway.

And simply asserting SAMPLEBUS everytime before reading J/KSTATUS is good practice seemingly. This is what MaximUSBLab10 code does.

My code works in my tests at least so far.

[Lauszus]

Okay. Let's leave it as is then :)

[Lauszus]

Sounds good to me, as the overhead is minimal.

[Lauszus]

Maybe add a comment why the code above was commented out and replaced with just busprobe.

[tmk]

Is this proper for the comment? If so I'll commit it.

// CONDETIRQ can miss detection of plugin and check bus every time anyway #653

[Lauszus]

What about something like this:

/* MAX3421 state change task and interrupt handler */
template< typename SPI_SS, typename INTR >
uint8_t MAX3421e< SPI_SS, INTR >::Task(void) {
#if 0
        uint8_t rcode = 0;
        uint8_t pinvalue;
        //USB_HOST_SERIAL.print("Vbus state: ");
        //USB_HOST_SERIAL.println( vbusState, HEX );
        pinvalue = INTR::IsSet(); //Read();
        //pinvalue = digitalRead( MAX_INT );
        if(pinvalue == 0) {
                rcode = IntHandler();
        }
        //    pinvalue = digitalRead( MAX_GPX );
        //    if( pinvalue == LOW ) {
        //        GpxHandler();
        //    }
        //    usbSM();                                //USB state machine
        return ( rcode);
#else
        // Seems like MAX3421e CONDETIRQ (connect/disconnect Interrupt) doesn't arise for every plugin event, 
        // so instead we check the bus state at every call to Task when no device is connected
        // See: https://github.com/felis/USB_Host_Shield_2.0/pull/653
        busprobe();
#endif
        return 0;
}

I think the above code makes it more clear what the old vs new code is as well.

[tmk]

It looks good!
Please feel free to add it as new commit or change existent commit before merging.

Thanks for your service on this library as always!