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

Working with ATtinyx61 #43

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Working with ATtinyx61 #43

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b93af7e
Working with ATtinyx61
tredpath Jun 26, 2021
74e0fdd
Update option name
tredpath Jul 9, 2021
94f7738
Check if PORT1 is to be used
tredpath Jul 9, 2021
3c2bcc9
Conditionally allow PORT1 to be disabled
tredpath Jul 9, 2021
0337774
Add disable for x61
tredpath Jul 9, 2021
2c7a9fc
Remove initPinChangeInterrupt
tredpath Jul 9, 2021
6b5d48e
Use true instead of 1
tredpath Jul 9, 2021
d129ff5
Remove initPinChangeInterrupt keyword
tredpath Jul 9, 2021
7ef2dbe
Rearrange disable helper for x61
tredpath Jul 12, 2021
4a28a87
getArrayPosPCINT returning incorrect value
tredpath Jul 12, 2021
4877616
Simplify disable logic
tredpath Jul 12, 2021
13219eb
Always clear interrupt masks on enable
tredpath Jul 12, 2021
e8bf710
Merge branch 'master' into working/attinyx61
tredpath Jul 12, 2021
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
24 changes: 24 additions & 0 deletions src/PinChangeInterrupt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,17 +210,34 @@ void disablePinChangeInterruptHelper(const uint8_t pcintPort, const uint8_t pcin
// disable the mask.
PCMSK0 &= ~pcintMask;

#if (defined(__AVR_ATtiny261__) || defined(__AVR_ATtiny461__) || defined(__AVR_ATtiny861__)) && (PCINT_USE_PORT1 == true)
// x61 devices uses the same interrupt enable for all of PCMSK0 (PCINT7:0) and the top half of PCMSK1 (PCINT15:12)
if (!PCMSK0 && !(PCMSK1 & 0b11110000))
GIMSK &= ~(1 << PCIE1);
#else
// if that's the last one, disable the interrupt.
if (!PCMSK0)
disable = true;
#endif
break;
case 1:
// disable the mask.
PCMSK1 &= ~pcintMask;

#if (defined(__AVR_ATtiny261__) || defined(__AVR_ATtiny461__) || defined(__AVR_ATtiny861__)) && (PCINT_USE_PORT1 == true)
// x61 devices use an interrupt specially for the bottom half of PCMASK1 (PCINT11:8)
if (pcintMask < (1 << 4)) {
if (!(PCMSK1 & 0b00001111))
GIMSK &= ~(1 << PCIE0);
}
// x61 devices uses the same interrupt enable for all of PCMSK0 (PCINT7:0) and the top half of PCMSK1 (PCINT15:12)
else if (!PCMSK0 && !(PCMSK1 & 0b11110000))
GIMSK &= ~(1 << PCIE1);
#else
// if that's the last one, disable the interrupt.
if (!PCMSK1)
disable = true;
#endif
break;
#ifdef PCMSK2
case 2:
Expand Down Expand Up @@ -253,12 +270,18 @@ void disablePinChangeInterruptHelper(const uint8_t pcintPort, const uint8_t pcin
if (*(&PCMSK + pcintPort) == 0)
disable = true;
#endif

//the special case for x61 devices will be handled above inline
#if !((defined(__AVR_ATtiny261__) || defined(__AVR_ATtiny461__) || defined(__AVR_ATtiny861__)) && (PCINT_USE_PORT1 == true))
if(disable)
{
#ifdef PCICR
PCICR &= ~(1 << (pcintPort + PCIE0));
#elif defined(GICR) /* e.g. ATmega162 */
GICR &= ~(1 << (pcintPort + PCIE0));
// if PORT1 is disabled on a x61 device this will handle the backwards PCIE definitions
#elif defined(GIMSK) && (defined(__AVR_ATtiny261__) || defined(__AVR_ATtiny461__) || defined(__AVR_ATtiny861__))
Copy link
Owner

Choose a reason for hiding this comment

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

I think the whole commit for disabling needs a rework. I think we only need a similar check to the enabling at this location, just for disabling. No need for this complicated logic. It looks error-prone.

Copy link
Author

Choose a reason for hiding this comment

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

I'll take a look again tomorrow but I think PCIE1 working with both ports requires the check to be a bit more complicated. The if will always need more checks than just PCMSK0 or PCMSK1 being 0 and inside the if (disable) block you would need to duplicate the pcintPort and pcintMask check.

Copy link
Owner

@NicoHood NicoHood Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This should do it:

#ifdef PCINT_COMBINE_PORT0_PORT1
#if defined(GIMSK) && (defined(__AVR_ATtiny261__) || defined(__AVR_ATtiny461__) || defined(__AVR_ATtiny861__))
    // This is a special case for Attiny x61 series which was very weird PCINT mapping.
    // See datasheet section 9.3.2:
		// http://ww1.microchip.com/downloads/en/devicedoc/atmel-2588-8-bit-avr-microcontrollers-tinyavr-attiny261-attiny461-attiny861_datasheet.pdf
#if (PCINT_USE_PORT1 == true)
    if (pcintPort == 1 && pcintMask < (1 << 4)) {
        // PCINT11:8 will be disabled with PCIE0
				if (!(PCMSK1 & 0x0F)) {
					GIMSK &= ~(1 << PCIE0);
				}
    }
    else {
        // PCINT7:0 and PCINT15:12 will be disabled with PCIE1
				if (!PCMSK0 && !(PCMSK1 & 0xF0)) {
#else
				if (!PCMSK0) {
#endif
					GIMSK &= ~(1 << PCIE1);
				}
#if (PCINT_USE_PORT1 == true)
    }
#endif
#else
#error MCU has no such a register
#endif

	// Always return, as we are using a different logic for disabling the interrupts.
	return;
#endif // ifdef PCINT_COMBINE_PORT0_PORT1

Place it above the if(disabled) section. It would be super nice, if you can give my code a review and also a real test on the device itself, as I cannot check. I did not even check if it compiles, as I've written it just in the editor.

Copy link
Author

Choose a reason for hiding this comment

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

I had to also do a bit of logic earlier to check the top and bottom half of PCMSK1 separately when deciding if disable should be set. If they are always checked together then there are cases where having interrupts enabled on multiple pins will stop PCIE0 and PCIE1 from being cleared when only one of the interrupts is disabled. And all code has been compiled and uploaded to a ATTINY861 with some interrupt tests using a button and some LEDs.

Copy link
Owner

Choose a reason for hiding this comment

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

I think you missunderstood my code. Now you have duplicated code. Why are you using the disabled flag at all? It does not really help in that special case. Thatswhy I suggested to ignore that and place my code above the if (disabled) check. Or is there any problem with that logic? I can imagine that it is faster and consumes less flash memory.

Copy link
Author

Choose a reason for hiding this comment

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

You're correct, I completely misread your comment. I've updated disablePinChangeInterruptHelper to remove the disable block for this case and just always check if the masks are empty.

GIMSK &= ~(1 << PCIE1);
#elif defined(GIMSK) && defined(PCIE0) /* e.g. ATtiny X4 */
GIMSK &= ~(1 << (pcintPort + PCIE0));
#elif defined(GIMSK) && defined(PCIE) /* e.g. ATtiny X5 */
Expand All @@ -267,6 +290,7 @@ void disablePinChangeInterruptHelper(const uint8_t pcintPort, const uint8_t pcin
#error MCU has no such a register
#endif
}
#endif // !((defined(__AVR_ATtiny261__) || defined(__AVR_ATtiny461__) || defined(__AVR_ATtiny861__)) && (PCINT_USE_PORT1 == true))
}

/*
Expand Down