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

low-level DMA serial #25

Open
wants to merge 3 commits into
base: master
from
Open

low-level DMA serial #25

wants to merge 3 commits into from

Conversation

@mmoskal
Copy link
Contributor

mmoskal commented Jul 9, 2019

I'm trying to communicate with ESP over serial using SAMDSerial. It kind of works at 115k but fails miserably at higher speeds. This is because the SAMDSerial is losing received bytes, which is not too surprising since it requires an interrupt to be handled on every character. The recv buffer on SAMD51 is 2 bytes, so at 2mbit we get 10us maximum interruption time, which isn't much.

It seems the Serial class in codal-core isn't really meant for DMA and I would much rather use the API of the SingleWireSerial, except I have two wires. (The other problem is that the regular serial isn't even implemented on STM, looking forward).

This adds capability for the second wire in SingleWriteSerial class. Maybe it should be LowLevelSerial or something, but that's detail. I think the STM class would be similarly easy to modify.

This also changes the DMA API to be more generic (Always. Pass. User Data. to callbacks).

It also fixes a bug with events not triggering.

@mmoskal mmoskal requested a review from jamesadevine Jul 9, 2019
* Base implementation of a DMA callback
*/
void DmaComponent::dmaTransferComplete(DmaCode) {}

DmaControllerInstance::DmaControllerInstance()

This comment has been minimized.

Copy link
@mmoskal

mmoskal Jul 9, 2019

Author Contributor

this just leads to trouble and everyone implements it anyways

This comment has been minimized.

Copy link
@jamesadevine

jamesadevine Jul 9, 2019

Contributor

Yeah, it should have been left at an abstract interface.

@jamesadevine

This comment has been minimized.

Copy link
Contributor

jamesadevine commented Jul 9, 2019

The standard serial abstraction is meant to make consumption of bytes easier in higher level languages and is not intended for fast communications with peripherals.

I'm really not keen on repurposing ZSingleWireSerial as the low level serial interface. The single wire serial class operates very differently to a full serial implementation and is designed to support the different communication duplexity: normal serial is Full duplex, and single wire serial is half duplex (you can either be in rx or tx mode, but not both). I'd much rather have a separate LowLevelSerial implementation, and then a class that augments LowLevelSerial to make it DMA compatible.

If you can wait a bit, I'll happily add core abstractions for this, but these changes really don't feel right to me.

@mmoskal

This comment has been minimized.

Copy link
Contributor Author

mmoskal commented Jul 9, 2019

We already duplicate the same code (and bugs) two times. Why do it three times? And why bother with non-DMA single write serial? Is anyone using that?

Also they don't seem to operate that differently since it took changing under 10% of the single wire class code to have it full duplex.

It should be easy to now use such a modified class in place of the legacy Serial in PXT.

@jamesadevine

This comment has been minimized.

Copy link
Contributor

jamesadevine commented Jul 9, 2019

Of course I agree that code duplication is bad

And why bother with non-DMA single write serial? Is anyone using that?

I agree that there should not be a separate DMA class for single wire serial, that was a bad decision.

Also they don't seem to operate that differently since it took changing under 10% of the single wire class code to have it full duplex.

The purpose of the single wire serial class was to support a single wire, not two. Code is about abstraction and clarity as much as it is about efficiency and optimality.

My point is that hacking the single wire serial class to do two wire serial was not the right approach. If the existing abstractions result in code duplication, then we should revisit the abstraction and create a class that supports both half and full duplex serial.

It seems like we're at least in agreement that the SingleWireSerial abstraction at least provides a good starting point for a conversation:

        // set mode logic etc. for configureRx/tx goes here..

        public:
        LowLevelSerial(Pin& p, Pin* optional, uint16_t id =XXX);
        virtual int setIRQ(void (*cb) (uint16_t errCode));

        virtual int putc(char c) = 0;
        virtual int getc() = 0;

        virtual int send(uint8_t* buf, int len) = 0;
        virtual int receive(uint8_t* buf, int len) = 0;

        virtual int sendDMA(uint8_t* buf, int len) = 0;
        virtual int receiveDMA(uint8_t* buf, int len) = 0;

        virtual int setBaud(uint32_t baud) = 0;
        virtual uint32_t getBaud() = 0;

        virtual int getBytesReceived() = 0;
        virtual int getBytesTransmitted() = 0;

        virtual int setCommunicationMode(SerialCommunicationMode m);
        virtual int sendBreak() = 0;
SerialCommunicationMode {
    FullDuplex,
    HalfDuplex
}

I think we may need both variants of send/receive because some classes may depend on one or the other.

@mmoskal

This comment has been minimized.

Copy link
Contributor Author

mmoskal commented Jul 9, 2019

we could have tx, rx in ctor and you just pass the same pin twice if needed.

I would call sendDMA/recvDMA as beginSend/beginRecv.

send/recv could be implemented in the base class with wait_for_event(), and I would drop putc/getc, not to encourage people to do such inefficient stuff.

if we could somehow add support for circular dma buffer we would be set

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.