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

reverse patch from d2aaebd #515

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

reverse patch from d2aaebd #515

wants to merge 1 commit into from

Conversation

gin66
Copy link
Contributor

@gin66 gin66 commented May 21, 2023

This patch reverses part of d2aaebd. If this reverse patch is not applied, then the library FastAccelStepper stops working on simavr. As FastAccelStepper works on the real device, simavr's behavior seems to be not correct and maybe that reverse patch is a fix.

@gatk555
Copy link
Collaborator

gatk555 commented May 30, 2023

This could be supported by an explanation of why it works. My guess is that the problem is that PWM output changes are not reflected in the corresponding PIN register. That can be reproduced by a simple test program (attached atmega324a_ioport.txt (C code but there is a stupid restriction on ".c" filename extensions.))

It sets up PWM output and prints the contents of the corresponding PIN register: after initialisation; after the first match event; after output has been enabled immediately following the match; and after the pin again changes state. The results should be 0, 0, 0x08, 0. With current git code they are 0, 0, 0, 0, so some changes in output are not reflected in the PIN register. With this PR: 0, 0x08, 0, 0. So this PR may fix one problem, but it will make spurious changes to PIN. With an alternative fix (patch.txt) that checks the state of DDR before copying PWM output to PIN: 0, 0, 0x08, 0x08. Still not right, but the last value is wrong because of a bug in the timer code. In my fork that is fixed and the correct result is shown.

gin66 added a commit to gin66/FastAccelStepper that referenced this pull request May 30, 2023
@gin66
Copy link
Contributor Author

gin66 commented May 30, 2023

Your patch works with my simavr based test suite. Later I will try your fork, which apparently is more active maintained.

gatk555 pushed a commit to gatk555/simavr that referenced this pull request Jun 1, 2023
PWM output from timer was not modifying the PIN register.
@buserror
Copy link
Owner

@gatk555 do you have a 'proper' PR or a commit for that? ie, c8bcdc7?

@gatk555
Copy link
Collaborator

gatk555 commented Aug 13, 2023

That is the change. I can not now recall how much testing I did against upstream code, but probably not enough. Does "future PR" sound OK?

@buserror
Copy link
Owner

Ahah fine :-)
As you've noticed I've reactivated my tree (mostly because I got an active AVR project!) so it's a good time to get stuff merged, if you hadn't given up ;-)

gatk555 pushed a commit to gatk555/simavr that referenced this pull request Dec 27, 2023
PWM output from timer was not modifying the PIN register.
buserror added a commit that referenced this pull request Feb 20, 2024
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.

None yet

3 participants