ArminJo / DigistumpArduino

Improved version of Digistump avr core for Arduino
194 stars 37 forks source link

Digispark Pro usbconfig.h #13

Closed SpenceKonde closed 3 years ago

SpenceKonde commented 3 years ago

Any idea why, with D+ on the INT0 pin of the T167 that the digispark pro uses, the usbconfig.h is telling it to use PCINTs for USB? Bootloader does this too - but no comment or hint to WHY. Presumably someone tried it the normal way and tripped over some weird behavior on that chip or something, but I'm sort of disturbed that they didn't comment on it.

I ask because - since it's gotta be on those pins since that's what the hardware you can buy for dirt cheap from china has connected, you'd be far better off using INT0 instead of wasting a whole bank of PCINTs like that....

Also, you can remove the old windows micronucleus package.

ArminJo commented 3 years ago

I only find the comment but must be configured as a pin change interrupt which in this case does not mean using a PCINT, it merely means using INT0 or INT1 but you can also use PCINT if you want.

Also, you can remove the old windows micronucleus package. What / where is this package?

SpenceKonde commented 3 years ago

So somewhere there was a comment that said that? It is done using a PCINT, in both the bootloader and the VUSB libraries - and I'm very curious about whether this actually is required - the fact that somewhere there was a comment that says that suggests that yes, it is required, and using INT0 does not work, for some weird reason!

Old file is the micronucleus-2.0a4-win.zip - the one youd deleted and then reuploaded after you found out that my core was using it. I've rehosted it and pointed my json file towards that.one, so I have no dependance on it anymore.

ArminJo commented 3 years ago

It is done using a PCINT, in both the bootloader and the VUSB libraries - and I'm very curious about whether this actually is required - the fact that somewhere there was a comment that says that suggests that yes, it is required, and using INT0 does not work, for some weird reason!

I liked to test INT0 instead of PCINT, but I give up since you have to modify void usbInit(void), to change also the MCUCR register. I guess, the main reasons for using PCINT are:

ArminJo commented 3 years ago

OK I finally tried it with

#define USB_INTR_CFG            MCUCR // requires 4 bit extra code size since sbi is not possible for MCUCR
#define USB_INTR_CFG_SET        (1 << ISC00)
#define USB_INTR_CFG_CLR        0
#define USB_INTR_ENABLE         GIMSK
#define USB_INTR_ENABLE_BIT     INT0
#define USB_INTR_PENDING        GIFR
#define USB_INTR_PENDING_BIT    INTF0

but at best I could burn the blink program, but for the 2. burn I got repeatedly Starting to upload ... >> Flash write error: Input/output error has occured" when a application is still loaded I checked twice with the same board and the same USB port, the PCINT version runs smoothly.

@cpldcpu Do you now why this is failing? The only difference for me is the interrupt prio.

cpldcpu commented 3 years ago

Hm... The reason to go for the pin change interrupt was to allow more flexibility, as far as i remember. Not sure why going back the the normal int would fail. Maybe there is a difference in how it is latched? MN uses the interrupt hardware in a rather nonconventional way, as you know.

SpenceKonde commented 3 years ago

@ArminJo I suspect part of why it didn't work with that configuration would be because the INT0 and INT1 interrupts are configured by the ISC00, ISC01, ISC10 and ISC11 bits in EICRA (External Interrupt Control Register A), not MCUCR, which only handles, it looks like, some BOD stuff and the global pullup disable)...

It looks like a slightly nicer setup than most classic AVRs for the INT1/INT0 interrupts. The 167 was one of the more capable tinyAVR parts - and unlike the "last three" classic tinies, they weren't (by all indications) racing against the closing "classic AVR release" window (the 828 in particular is very sad - it looks like it was going to have a MUCH fancier ADC, like the one in the 841, only with 28 input channels... but the bits that might control that are marked reserved. Datasheet was clearly initially written to describe the differential ADC, and then that was cut (there are references to gain, and the single ended channels are referred to as if there's another kind.... (didja know that the 167, as well as the 841 amd 1634, can switch system clock source at runtime? (the 828 also is the kind of thing where you really wonder who is buying enough to keep the supply line running , and why... I suspect the answer involves the wreckage of a very fancy ADC that has silicon errata that is too extensive for microchip to publically associate themselves with it... implying something so severe that it can

More flexibility? Hmmm I'm not sure I see how grabbing a whole port of PCINTs away from the user is more flexible than just using the one... but maybe I'm not fully understanding this...

cpldcpu commented 3 years ago

So you are not talking about micronucleus here? MN does not take anything away from the user. Regarding the digispark libraries - no idea.

ArminJo commented 3 years ago

Here I talk only about micronucleus here, I have no clue with the V-USB in the libraries... To clarify, I set the ISC00, ISC01 bits to generate an interrupt on both edges, which increases the bootloader size a bit. It works perfectly until the write stage.

SpenceKonde commented 3 years ago

I noticed it while gathering the USB settings from all the MN-boards into one file (part of my plan to minimize how much they'll suck to work with in ATTinyCore 2.0.x). And I saw PCINT and was like "wtf, it's on INT0, why would they hose the whole port worth of PCINTs instead of using INT0. which I thought was supposed to be preferred anyway" and then checked MN - and its done that way there too... which made me wonder why that choice was made and if there is some obscure problem with INT0 - it sounds like there may be,

cpldcpu commented 3 years ago

But that does not matter at all. The user program can simply change the interrupt configuration. MN does not take any ressources from the user program.

SpenceKonde commented 3 years ago

Of course - but the reason I asked about this was for the libraries, and I was wondering whether there was a problem with using INT0 since they both did it via PCINT, which seemed surprising