Update AMDS Firmware for Timing Manager in AMDC #48
Conversation
|
Hey @codecubepi , this PR needs the following before review & close:
|
There was a problem hiding this comment.
Thanks @codecubepi.
One comment for us to discuss---I know this code works, has been tested pretty extensively, etc. So, I hesitate to change anything. However, if you read the ADC code, it has some extra complexity with two memory buffers that it ping-pongs between due to the old ISR operation. However, now, since there is only 1 ISR context that does everything, it does not need this complexity. It would be nice to simplify the code a bit, but I am not sure we need to invest the time to do that, test it, etc.... what do you think?
| // Switch read buffer to where we just put the new data, | ||
| // therefore, satisfying the property that the read_buffer_number | ||
| // always points to a valid set of samples! | ||
| read_buffer_number = 1 - read_buffer_number; | ||
|
|
||
| // Now that read buffer is set, we are safe to update write buffer | ||
| // for the next time this ISR runs... | ||
| write_buffer_number = 1 - write_buffer_number; |
Well, if I were to fix the code to address the tab/space issue, I might as well just remove the unnecessary buffer code (and do some retesting). Otherwise it'd be easy to open an issue and have a be a "good first issue" for future contributors... but in all reality it could turn into an issue that never really gets addressed... I think the first option of me just fixing it and retesting is probably the better way to go 🙃 |
|
I took care of removing those obselete double buffers. I ran a hardware test with the fixed ADC data flushing, to verify that the AMDS data still matches what is seen by the AMDC's on-board ADC:
|
There was a problem hiding this comment.
So nice to have clean code :) Thanks @codecubepi
Best code is no code---115 lines removed!
Haha you're welcome. I can delete more if it'll make you even happier 😂 |
|
Note from @elsevers : We should start tagging AMDS releases with a version number that matches the AMDC Firmware version number, so user's know when firmware is compatible. So the previous commit on |
|
Per the release of AMDC-Firmware |
Yes, let's merge this! See my comment above about version tagging in this repo to match AMDC-Firmware. |
@codecubepi, I compared notes with @npetersen2 on this and we decided on another approach to handle this. From @npetersen2 's message:
|
|
Yeah, this is a completely fair concern. I'm willing to concede 😄 |
This PR is a mild revision to the AMDS Firmware that makes its side of the interface compatible with the new AMDS driver and new Timing Manager IPs introduced in v1.3 of the AMDC Firmware.
This is a relatively small code change on the AMDS firmware side. The code that is called to transmit the data from the AMDS to the AMDC driver over UART was previously in an interrupt context. Now that the data transmission is meant to happen as soon as all the daughtercards have been sampled, this code is no longer executed by its own interrupt.
Therefore, the UART transmission ISR has been moved into the function "
transmit_samples()", which is called from the ADC sync interrupt context after sampling is completed.Making this change also allowed me to remove the "ignore next sample" issue, since the sampling will no longer go out-of-sync with the PWM carrier due to data transmission.
I still need to update docs.amdc.dev page on the AMDS' firmware. I will do so once we complete v1.3 in the AMDC repo.