-
Notifications
You must be signed in to change notification settings - Fork 64
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
Bulk transfer SPI function #700
Conversation
Again great work. SPI would be my first priority since bulk transitions are required for SD cards, displays, ethernet, etc.. I will try, in these next couple of days, add proper support for this inside the module ecosystem and test it with a module. Thanks. |
Hi there. DETAILSIn the early days you could create software emulated spi ports using generic pins or call the mcu spi port if available either directly or via the softspi functions passing a NULL softspi port struct. One of uCNC main ides is to build a set of HAL libraries that allow features and modules to be supported by all architectures. For that reason softspi and the mcu spi HAL was recently changed to also try to gap the difference in Arduino implementations for each architecture and allow to create a SoftSPI port over an Arduino SPI port for any given architecture. In light of this and pressured by your very appreciated help I quickly drafted a modification to the whole spi thing to accomodate this. Here are my takes and this:
This would work for most cases I believe either from an MCU HAL perspective and a module/library perspective side. Hope to ear from you soon. |
Thanks for the feedback, I'm glad I can be of help. Your suggestions look fine to me, however wouldn't polling and running main loop in Edit: |
There is no issue calling it from a main loop module because modules event listeners call are not re-entrant. If an event listener of the main loop calls the main loop it self it will run the main loop on a new stack but it will not call it self. It's the RTOS-less solution I found for µCNC to be able attach multiple executions to the same calling point in the core code. Somewhat like event driven tasks that are spawned in the main super loop. There are also some other type of configurations that can be tested (and I may make them optional in the future) to give mode CPU time to more consuming time tasks. The main thing is that the About Yes the #701 because I did it in a hurry to avoid you losing time building stuff that may not be necessary. |
Just some further clarification. Most event handlers behavior are implemented with this macro This follows a simple linked list of event listeneres and if they are not marked as already running, executes them. It also tries to give even access to the event, by always continue from the last event listener that was called in the previous handler loop. This allows for a certain degree of inception that gives the illusion a multithread environment while maintaining the predictability of the MCU IO ISR's without an RTOS as a middleman. The only exception of course is the ESP chips since there is no usable work around. Edit |
Removes bulk transfer definition from mcu.h, will be reintroduced PR Paciente8159#701
Changed spi bulk transfer function to fit the given specification and usage in softspi bulk transfers.
One final detail that just came to my mind. With this type of use of the SPI multiple race conditions can occur between modules fighting for access to the HW SPI port. Let's say To prevent this I need to add a lock at the generic |
I think placing it in softspi as part of the |
I agree with you. I'm also inclined to think that maybe the HW side of the SPI implementation might need an is_busy kind of function to make it easier to implement, instead of looping over the bulk send. What is you opinion? The thing is that softspi is a complementary module. I will mirror it probably. |
I'm not sure honestly. In my implementation of bulk transfers I have a couple of checks for the availability of hardware resources which return back to looping if they are busy. Splitting the function up into two parts would mean that we would first have to poll the 'transfer_start' function and then 'is_busy'. I'd just leave it as it is and make the HW side deal with it. |
I've uploaded the changes to the resource access locking via softspi. There is a softspi_config_t that uses one of the reserved bits to implement the lock. |
Implements bulk transfer for AVR MCUs using the serial transfer completed interrupt.
I've implemented bulk transfers for MCU architectures which I am familiar with. Tomorrow I will try to write an implementation for LPC176x and SAMD21 (without any tests as I do not have access to these boards). ESP32, ESP8266 and RP2040 will only get a simple implementation using single byte xmit and timeout. |
Awesome work. This is great. Regarding other architectures, there is no need to do anything. |
I've modified a version of the SD card module to support bulk transfers but did not tested, not even for compilation errors. One question popped to my mind. Perhaps the best approach is simply for every implementation of bulk transmit (both HW and SW) be a blocking call that as internal logic to call the main loop. That means that a transfer via DMA is done and also blocks while it waits for it to finish looping over the main task loop. EDIT Nevermind.. I've took a closer look to your implementation and missed the status flags. I also like the way you keep track of the data pointer. I will modify #701 to reflect this. 👌 |
Actually you are right, the functions do mostly keep track of the status but single byte transmissions don't check that flag. I'll make sure to fix that. |
When I said it didn't matter it's because via softspi bulk call will only return upon bulk transmission is complete. So that situation would never happen.. unless you use the mcu hal spi calls directly. But that scenario should be avoided anyway. The correct way to do it is always via softspi and soft spi guards against that. |
I've added code a (untested) code to support bulk transfers over ESP32 on #701 . ESP seems to do alot of hidden stuff under the hood, so I'm curious if it will translate in terms of performance. |
Helpful SPI port definitions for some of the MCUs
Alright, I won't actually be doing LPC176x because I don't quite understand how its DMA works. Now after you merge #701 I will update everything to work with the new configuration struct and mark this PR as ready to review. |
Huge thank you. EDIT |
I've drafted a DMA and non DMA implementation on LPC17xx MCU. After some analysis and some thought on the matter I came to realize you were correct. There are situation were only transmission is required. One of these situation is exactly on the SD card, when saving a disk page, you are only interested in sending 512 bytes of data, without care about what is being read. I'm sorry for all this trouble.
to
That way you can confirm if it's a tx only operation if the I will proceed to modify #701 to match this. |
Done |
I've successfully implemented a working SPI bulk transmission with or without DMA on ESP32 with a considerable performance gains. On LPC17xx not much success. Using the generic polling implementation shows very good speeds, but I'm yet to implement the DMA version. |
@patryk3211 I've done the work on #701 and is more or less ready to be integrated. |
Yes you can, the code should be finished |
You'll need to alter the |
I will proceed to test your code hover this branch. If it's all ok then I will integrate both into master. |
Tested SAMD21. Generic mcu bulk transfer works as expected. |
I don't know if I'll be able to help much since I don't have access to the hardware but can you check out at which stage it fails? Does transmission even start, if it does, are all bytes being sent? |
Don't worry I will try to figure out. EDIT I've managed to fix the ISR code on SAMD21. SAMD21 DMA implementation seems to be a bit more extensive then the ones I tackled so I still need to digest the datasheet for a bit to see if I can make some sense out of it. I will repick work tomorrow. |
Like in the LPC I only got DMA transfers to work when tx only mode is used. I made some refactoring of your code to a much more simple scenario using static channels (no channel availability checking) and static priority (no round robin). Tomorrow I will see how it goes for the STM32 MCU's. |
This modification seems to work without lockout. The following changes were implemented - instead of dynamic DMA channel allocation it uses a static channel for RX and other for TX - manual priority strategy based on channel number instead of round-robin - code cleanup and simplification
I've pushed a modified version of SAMD21 with the simplified version of the DMA SPI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not able to test STM32 modifications. I'm going to merge the changes since you are already using them to control display graphics.
I will add an option to bypass custom MCU implementations and use the software to allow user to test alternative method in case of problems.
This pull request will implement bulk transfer operation for the SPI interface which depending on the MCU's capabilities will utilize DMA and allow for asynchronous data transfer.