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.

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

avr: adding pin change interrupt handling to atmega328p #3139

Closed
wants to merge 7 commits into from

Conversation

mrbell321
Copy link

This is what I've come up with for AVR pin change interrupts.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Thanks, we need this! However, this PR is not ready for review: it contains tons of debug code. Can you please clean up the PR?

src/machine/machine_atmega328p.go Outdated Show resolved Hide resolved
src/machine/machine_atmega328p.go Outdated Show resolved Hide resolved
@aykevl aykevl mentioned this pull request Sep 8, 2022
@mrbell321
Copy link
Author

I created a ticket for this work too: #3145
Not sure what the official ticket/git tagging mechanism is...

src/machine/machine_atmega328p.go Outdated Show resolved Hide resolved
src/machine/machine_atmega328p.go Outdated Show resolved Hide resolved
src/machine/machine_atmega328p.go Outdated Show resolved Hide resolved
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

This looks good, with some nits here and there that could improve the code a bit and make it more efficient. Thank you for your continued work on this PR!

src/machine/machine_atmega328p.go Outdated Show resolved Hide resolved
src/machine/machine_atmega328p.go Outdated Show resolved Hide resolved
src/machine/machine_atmega328p.go Outdated Show resolved Hide resolved
Comment on lines 519 to 523
PCMSK_REG = avr.PCMSK0
PCIE = avr.PCICR_PCIE0
PIN0 = PD0
PCINT = 1 << (pin - PIN0)
IRQ = avr.IRQ_PCINT0
Copy link
Member

Choose a reason for hiding this comment

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

You could consider the following:

Suggested change
PCMSK_REG = avr.PCMSK0
PCIE = avr.PCICR_PCIE0
PIN0 = PD0
PCINT = 1 << (pin - PIN0)
IRQ = avr.IRQ_PCINT0
pinIndex := pin - PD0
pinCallbacks[0][pinIndex] = callback
avr.PCMSK0.SetBits(1 << pinIndex)
avr.PCICR.SetBits(avr.PCICR_PCIE0)

It's less abstract, but it's actually a bit shorter and perhaps more readable as well. Also, I think the compiler will be better able to optimize this code.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I was intentionally being abstract because... well... reasons.
However, it ended up not working out very well that way due to... other reasons.
This new way simplifies quite a bit so I'll have the MR updated soon.

...I also need to figure out why "make release" is now crashing my machine....

Copy link
Author

@mrbell321 mrbell321 Sep 22, 2022

Choose a reason for hiding this comment

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

So, I've make release, at least to the point that it runs to completion by ripping out all of tinygo and golang and reinstalling them.
However, I now get

$ tinygo build -target=arduino   
warning: no avr-gcc installation can be found on the system, cannot link standard libraries
warning: standard library not linked and so no interrupt vector table or compiler runtime routines will be linked
warning: no avr-gcc installation can be found on the system, cannot link standard libraries
warning: standard library not linked and so no interrupt vector table or compiler runtime routines will be linked

However, I have avr-gcc

$ avr-gcc
avr-gcc: fatal error: no input files
compilation terminated.

Copy link
Member

Choose a reason for hiding this comment

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

Please see https://tinygo.org/getting-started/install/linux/#avr-eg-arduino-uno for the install requirements for AVR on Linux.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Just one last thing

src/machine/machine_atmega328p.go Outdated Show resolved Hide resolved
@deadprogram
Copy link
Member

So @mrbell321 has this been tested on real hardware?

@mrbell321
Copy link
Author

mrbell321 commented Sep 29, 2022 via email

@mrbell321
Copy link
Author

mrbell321 commented Sep 29, 2022 via email

switch {
case pin >= PB0 && pin <= PB7:
// PCMSK0 - PCINT0-7
pinIndex := pin - PD0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pinIndex := pin - PD0
pinIndex := pin - PB0

@ysoldak
Copy link
Contributor

ysoldak commented Jun 7, 2023

I've tried this change on a custom board with atmega328p chip.

Interrupt fires. Can't use irremote driver directly though, since this change misses PinRising and PinFalling states.
I'm not exactly sure how, but handling of the state shall be possible to add, see for example https://www.avrfreaks.net/s/topic/a5C3l000000URmcEAG/t120592

@ysoldak
Copy link
Contributor

ysoldak commented Jul 31, 2023

This can be closed now, @deadprogram ?

@deadprogram
Copy link
Member

@ysoldak yes.

@mrbell321 thank you for pointing the way on the implementation. Now closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants