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.

Sign up for GitHub

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

Improve plugin detection in busprobe() #653

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve plugin detection in busprobe() #653

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions usbhost.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,9 +503,28 @@ int8_t MAX3421e< SPI_SS, INTR >::Init(int mseconds) {
/* probe bus to determine device presence and speed and switch host to this speed */
template< typename SPI_SS, typename INTR >
void MAX3421e< SPI_SS, INTR >::busprobe() {
static uint8_t prev_bus = -1;
uint8_t bus_sample;

if (regRd(rHIRQ) & bmCONDETIRQ) {
// device plug/unplug is detected
// clear CONDETIRQ
regWr(rHIRQ, bmCONDETIRQ);
} else {
// check current bus state when no device(SE0),
// otherwise transient state will be read due to bus activity.
if (vbusState != SE0) return;

// read current bus state
regWr(rHCTL, bmSAMPLEBUS);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 "); };

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}

bus_sample = regRd(rHRSL); //Get J,K status
bus_sample &= (bmJSTATUS | bmKSTATUS); //zero the rest of the byte

if (prev_bus == bus_sample) { return; }
prev_bus = bus_sample;

switch(bus_sample) { //start full-speed or low-speed host
case( bmJSTATUS):
if((regRd(rMODE) & bmLOWSPEED) == 0) {
Expand Down Expand Up @@ -538,6 +557,7 @@ void MAX3421e< SPI_SS, INTR >::busprobe() {
/* MAX3421 state change task and interrupt handler */
template< typename SPI_SS, typename INTR >
uint8_t MAX3421e< SPI_SS, INTR >::Task(void) {
/*
uint8_t rcode = 0;
uint8_t pinvalue;
//USB_HOST_SERIAL.print("Vbus state: ");
Expand All @@ -553,6 +573,9 @@ uint8_t MAX3421e< SPI_SS, INTR >::Task(void) {
// }
// usbSM(); //USB state machine
return ( rcode);
*/
busprobe();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

return 0;
}

template< typename SPI_SS, typename INTR >
Expand Down