Fixes #103: Misleadingly 'else' clause

[fubian]

No description provided.

[matthijskooijman]

Looks good at first glance. Two remarks:

• I think fixing the indentation might also be sufficient fix, from a quick look the coding style of this file seems to be to omit braces for single-statement ifs, so maybe adding them there here is not ideal.
• Did you check the code to see if this is actually the correct code? It could be that the else was intended to contain both statements (in which case there is a bug that might need to be fixed, instead of incorrect indentation).

[fubian]

The coding style is inconsistent anyways: compare the following examples.

• see if clause in line 250
• see if clause in line 287
• see if clause in line 398

I replaced now all the tabs with four spaces.

The following snipped from line 668 to line 712 illustrates in the last else clause that the two consecutive UDD_Send8(EP0, 0); commands shall be within the same scope (line 708 and line 709).

if (GET_STATUS == r)
{
    if( setup.bmRequestType == 0 )  // device
    {
        // Send the device status
     	TRACE_CORE(puts(">>> EP0 Int: GET_STATUS\r\n");)
        // Check current configuration for power mode (if device is configured)
        // TODO
        // Check if remote wake-up is enabled
        // TODO
        UDD_Send8(EP0, 0); // TODO
	UDD_Send8(EP0, 0);
    }
    // if( setup.bmRequestType == 2 ) // Endpoint:
    else
    {
        // Send the endpoint status
        // Check if the endpoint if currently halted
        if( isEndpointHalt == 1 )
    	UDD_Send8(EP0, 1); // TODO
        else
    	UDD_Send8(EP0, 0); // TODO
	UDD_Send8(EP0, 0);
    }
}
else if (CLEAR_FEATURE == r)
{
    // Check which is the selected feature
    if( setup.wValueL == 1) // DEVICEREMOTEWAKEUP
    {
        // Enable remote wake-up and send a ZLP
        if( isRemoteWakeUpEnabled == 1 )
	UDD_Send8(EP0, 1);
        else
	UDD_Send8(EP0, 0);
        UDD_Send8(EP0, 0);
    }
    else // if( setup.wValueL == 0) // ENDPOINTHALT
    {
        isEndpointHalt = 0;  // TODO
    	UDD_Send8(EP0, 0);
	UDD_Send8(EP0, 0);
    }
}

Due to that, I corrected the placement of the braces.

[matthijskooijman]

Seems that replacing tabs by spaces is a good idea, that file has mixed tabs and spaces which is hardly ever a good idea.

The coding style is inconsistent anyways: compare the following examples.

Seems you looked further than I did, so fine to leave it like this.

The following snipped from line 668 to line 712 illustrates in the last else clause that the two consecutive UDD_Send8(EP0, 0); commands shall be within the same scope (line 708 and line 709).

I'm not so sure I follow this. Do you mean because there are also two UDD_Send8 in the ENDPOINTHALT case, so you expect to be sending two bytes rather than one? If so, that argument actually argues against putting both sends in the else. You now have this: https://github.com/fubian/ArduinoCore-sam/blob/50ee941415f6c39f31972f5259ba75feaea16f11/cores/arduino/USB/USBCore.cpp#L703-L711

However, that sends 1 or 2 bytes, depending on the value of isRemoteWakeupEnabled, which I'm not sure is correct. To really know what is correct, the USB spec should be consulted.

So I did (I looked at the main USB spec, version 2.0, April 27, 2000). For GET_STATUS, it should always return 2 bytes, as indicated by the wLength field:

The returned values for the case handled here, are indeed just a halt bit:

So, your latest push seems incorrect, since it sometimes returns just one byte.

For CLEAR_FEATURE, something weird is going on. The specification says that no bytes should be returned, as indicated by a wLength of 0:

Maybe I'm misunderstanding what UDD_Send8 means exactly, but it seems like CLEAR_FEATURE is not being handled properly (though maybe some lower level handling checks the wLength field and just silently drops these 2 bytes, dunno). Also, it seems that the isRemoteWakeupEnabled field is not actually cleared. It can be set, but not queried and is also not actually used anywhere, so I guess this is just unfinished implementation that we can just ignore at least in the scope of this PR.

Finally, as for the structure of your commits, you have now mixed a functional change (moving UDD_Send8 calls into else clauses) with a very big mechanical commit (replacing tabs with spaces), which makes it pretty much impossible to spot the difference. These changes should really have been be separated (though it seems the moving into the else should be reverted in any case, so that might not be so much of an issue).

Ideally, you would get two commits:

1. Replace all tabs with spaces (and nothing else)
2. Fix indentation on these two if/else clauses.

This requires some git history editing (e.g. with rebase) and a force push (please do not open a new PR for this). Let me know if you can't figure this out (git is not the most friendly out there), then I can also fix this for you.

[fubian]

Ok. Thanks for your detailed review! I sorted my commits and corrected the UDD_Send8(EP0, 0) issue of my previous suggestions.

[matthijskooijman]

Perfect, looks good to me like this. I can't actually merge this, so I'll approve it and maybe @cmaglie or @facchinm can merge (might be a while until they get around to it, though).