pbsys/storage: Move loading and saving of user data to pbdrv/block_device #322
Conversation
dlech
left a comment
dlech
left a comment
There was a problem hiding this comment.
Nice to see this stuff getting pushed down to the driver level. 😄
| void pbdrv_adc_ev3_configure_data_format() { | ||
| // Public init is not used. | ||
| void pbdrv_adc_init(void) { | ||
| } |
There was a problem hiding this comment.
Would make sense to at least have pbdrv_init_busy_up(); here since this is deferred init.
| PBIO_OS_ASYNC_END(PBIO_SUCCESS); | ||
| // ADC may start polling now. | ||
| #if PBDRV_CONFIG_ADC_EV3 | ||
| pbdrv_adc_ev3_init(); |
There was a problem hiding this comment.
This works, but a more general solution would be to have a condition set and condition wait and just have the ADC driver wait for the require condition rather than having this driver be so knowledgeable about the ADC driver.
| pbdrv_init_busy_down(); | ||
|
|
||
| PBIO_OS_ASYNC_END(PBIO_SUCCESS); | ||
| // ADC may start polling now. |
There was a problem hiding this comment.
| // ADC may start polling now. | |
| // ADC may use the SPI bus now. |
| pbdrv_adc_ev3_configure_data_format(); | ||
| #endif | ||
|
|
||
| // Additional SPI configuration ADC is done when the ADC driver starts. |
There was a problem hiding this comment.
Ideally the ADC driver would just reconfigure everything rather than depending on this driver having intimate knowledge of the ADC driver. This way, we can tweak things here without worrying about breaking the ADC driver.
| * │ user program(s) ▼ to flash │ user RAM | ||
| * │ │ | ||
| * │ remaining user ram: application heap ▼ | ||
| * │ remaining application heap ▼ |
There was a problem hiding this comment.
For later consideration: on EV3, it would make sense to just have a separate heap so that the heap size is always the same since we have so much memory.
👍
Thinking about this some more, it occurred to me that the opposite of this could make sense as well. Since they depend on the same peripheral, it seems like splitting them could give the appearance of independence when that isn't really the case. So rather than splitting them, perhaps they could go in a single implementation. (It would still follow the logic introduced here, just refactored with no artificial hooks across them.) |
The way I see it, they are quite independent. Both drivers have exclusive control of the SPI controller/bus, just not at the same time. So each driver should be able to do whatever it needs to do to the SPI controller without having to worry about the other driver. The only thing each driver needs to know is, "do I have exclusive access of the SPI controller or not?" When reviewing the code, it was hard enough for me to keep in my head what the SPI controller needed to do for one driver, let alone two, so I'm not particularly keen on anything that tries to combine them. |
|
Yeah, so seeing them in a single process that loads SPI flash, polls ADC until shutdown and then writes to flash could make it potentially quite easy to follow since everything is one place without the need for access control. That said, I'm certainly not against splitting them and adding some higher level logic to manage access. 😄 Since it's separate from this PR, so I'd like to defer doing that to the next task. |
|
Added commits to drop both the I'm planning to apply your suggested changes and then merge it. Then we can look at splitting the two SPI drivers properly as well as discussed above. |
lib/pbio/src/port_dcm_ev3.c
Outdated
| PBIO_OS_AWAIT_MS(state, timer, 2); | ||
| PBIO_OS_AWAIT(state, &dcm->child, pbdrv_adc_await_new_samples(&dcm->child)); |
There was a problem hiding this comment.
Is the extra 2 ms wait for settling time? I.e. if we were to take a ADC reading immediately after changing the LED, would it be the wrong value?
There was a problem hiding this comment.
It's probably a lot smaller for sure, but 2 ms is the shortest we can currently guarantee to be non-zero.
I've revisited it now so that it uses only the time of the SPI DMA completion event instead of the systick event. Then we can just compare a small us time (except that pbdrv_clock_us isn't implemented yet).
This moves the platform-specific checksum and saving logic to the driver level. This is a better fit generally, and it is needed for EV3 where the flash shares the SPI bus with the ADC driver. When both reside at the pbdrv level, they can run sequentially without holding up other drivers. This also renames the public functions to more accurately represent what they do. This can only be used to save the disk in one go, so we can't call them generic storage functions. Fixes pybricks/support#2264
The block device initiates the ADC polling process when it is fully loaded, and awaits the ADC process to exit before it writes. Fixes pybricks/support#2264
Now that the ADC uses DMA, there is little cost in just polling faster. This way we don't end up polling in a tight loop when many NXT light or color sensors are attached. Also add a way to await new samples so we don't have to hardcode delays at higher levels.
Most process that use the ADC are slow and don't need to get called back every other millisecond now that we increased the sample frequency. They can poll it at their own reduced independent frequency, as we do in most device detection loops.
Also separates the ramdisk logic from the actual SPI read and write operations. This helps to understand the shared SPI code better. It also becomes simpler to split them properly if we choose to do that later.
This is not only useful for motor control code.
We yielded and polled immediately after initiating the ADC transfer to give other awaitables a chance to start waiting on the transfer. This leads to many extra events and could theoretically introduce a race condition if the DMA operation completes before it gets into the right wait state. Instead, higher level code can await new samples by waiting until the DMA completion timestamp has passed the time of interest, without inducing more events. While we do this, we can conveniently introduce a handle to wait until a specified future sample, which can be used if signals need to settle before measuring the ADC value.
See pybricks/support#2264 for discussion and rationale.