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

Add support for the acknowledge pin #13

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

Conversation

mthuurne
Copy link

@mthuurne mthuurne commented Jul 4, 2021

The acknowledge pulse can be very short: 4.5 μs on my 32u4 board, 3 μs on my PAL PSX. Therefore polling the pin risks missing the pulse. However, by using a masked external interrupt we can monitor the pulse reliably.

The acknowledge pulse can be very short: 4.5 us on my 32u4 board, 3 us
on my PAL PSX. Therefore polling the pin risks missing the pulse.
However, by using a masked external interrupt we can monitor the
pulse reliably.
This combination might be useful for testing.
@mthuurne
Copy link
Author

mthuurne commented Jul 4, 2021

Is it OK to configure hardware in the constructor or should that be moved into begin()? And if so, should the argument also be moved or should that be stored by the constructor or made into a template argument?

Would it be better to add the interrupt flag technique to the DigitalIO library instead of in PsxNewLib itself?

@SukkoPera
Copy link
Owner

Please avoid arguments in c'tors. I think the ack pin can be made a template argument like the other pins. I think I did it that way in the devel branch.

The interrupt flag can be left in PsxNewLib. My only concern would be that using only pins capable of HW-Int is a bit limiting. I think I did it with pin-change interrupts in the devel branch but I'm not sure, it's been some time.

Anyway as long as it stays optional, it's a great feature that allows for more reliable communication, so thanks for your contribution. Let me know when it's complete.

@mthuurne
Copy link
Author

mthuurne commented Jul 4, 2021

Please avoid arguments in c'tors. I think the ack pin can be made a template argument like the other pins. I think I did it that way in the devel branch.

I'll rework the code.

I'm a bit surprised that NOT_A_PIN has value 0 rather than -1, since some boards do have a pin 0. Maybe I should be using -1 as the default value for the ack pin instead?

The interrupt flag can be left in PsxNewLib. My only concern would be that using only pins capable of HW-Int is a bit limiting. I think I did it with pin-change interrupts in the devel branch but I'm not sure, it's been some time.

I'm using a clone of the SparkFun Pro Micro, which is like the Arduino Leonardo. On this board external interrupts are available for pins 0, 1, 2, 3, 7 while pin change interrupts are available for pins 8, 9, 10 (as pin 11 and 17 are not accessible and pin 14, 15, 16 are in use by SPI). So external interrupts actually provide more options for this particular board.

@mthuurne
Copy link
Author

mthuurne commented Jul 4, 2021

I avoided the NOT_A_PIN issue by adding a subclass, similar to what you did on the devel branch.

Let me know when it's complete.

I think the implementation is complete now. Maybe the docs and examples need some changes so people will know the feature exists.

@mthuurne
Copy link
Author

mthuurne commented Jul 4, 2021

Would it be safe to wait for the falling edge of ACK instead of the rising edge? That would reduce latency a bit, especially if the pull-up on the ACK pin is weak.

@SukkoPera
Copy link
Owner

Pull-ups MUST be 1k. The library is not guaranteed to work with larger ones.

What I did in the devel branch is waiting for the falling edge of ACK and then for the line to get back high. If you just wait for a rising edge you can probably have less code and work just as well.

I would not just wait for the falling edge, that doesn't sound good to me. Latency is already much improved wrt not using the ACK pin at all. If you really wanted to spare that couple of microseconds, I guess you can introduce a new "fast but maybe unreliable" derivation of PsxController.

@mthuurne
Copy link
Author

mthuurne commented Jul 5, 2021

Pull-ups MUST be 1k. The library is not guaranteed to work with larger ones.

I already soldered 5k pull-ups, which seem to work for my board at least. If I ever do another one, I'll use 1k, but I don't want to risk breaking my working board by re-soldering.

The ACK pulse being 50% longer (4.5 μs instead of 3 μs) on my board is likely due to the 5k pull-up though. It was 15 μs when using only the internal pull-up of the 32u4.

What I did in the devel branch is waiting for the falling edge of ACK and then for the line to get back high. If you just wait for a rising edge you can probably have less code and work just as well.

Yes, waiting for the rising edge is what is currently implemented in this PR and that part required relatively little code. The biggest change I had to make was to make sure it doesn't wait for ACK after receiving the last byte.

I would not just wait for the falling edge, that doesn't sound good to me. Latency is already much improved wrt not using the ACK pin at all. If you really wanted to spare that couple of microseconds, I guess you can introduce a new "fast but maybe unreliable" derivation of PsxController.

If it's unreliable, it would not be worth saving a few microseconds. However, I don't know which flank of the ACK pulse is intended to be sampled: if the controller is ready at the falling flank, there would be no need to wait for the rising flank.

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.

2 participants