On Tue, Sep 4, 2018 at 6:55 PM Ashish Chaudhari <ashish.chaudh...@ettus.com> wrote:
> On Tue, Sep 4, 2018 at 8:07 AM, Brian Padalino via USRP-users > <usrp-users@lists.ettus.com> wrote: > > Recently there was a significant change to the noc_block_vector_iir > > (specifically the vector_iir): > > > > > > > https://github.com/EttusResearch/fpga/commit/05ca30fe91330d54dcd005a3af4aeaa0dc26c8f8#diff-4b21f52231ba9f82bf132f633d594187 > > > > It's a pretty significant re-write of the block, and this behavior has me > > asking a few questions: > > > > 1) Can anyone at Ettus expand on the reason for the major re-write? It > > appears that the block itself worked previously so it seems like > re-writing > > it was a lot of effort to potentially add new bugs to things which > weren't > > there previously. > > > > There were several reasons that contributed to the re-writing that > block. First off, there was a bug in the delay line implementation > that needed to be fixed. Secondly, we wanted to improve the > readability and resource utilization of blocks where users hand-write > their DSP (as opposed to using third-party AXI-Stream IP). The old > approach was to implement a DSP algorithm using basic operations like > addition, multiplication, rounding, etc each with AXI-Stream > handshaking. This has the following drawbacks: 1) The code is a bit > hard to read because the AXI-Stream stuff takes up code real-estate 2) > It's hard to let the tools infer DSP primitives automatically because > AXI-Stream can get in the way of the standard Xilinx-intended control > sets. 3) The AXI-Stream logic takes up more fabric resources. The > Vector IIR block is the first block that wraps AXI-Stream around the > entire DSP algorithm instead of around basic functions. This is the > way we intend to write future blocks with hand-implemented DSP. The > change is actually a step forward in terms of stability because the > API to the actual DSP kernel is much simpler and intuitive, and is > much closer to how FPGA DSP implementation is taught. The simpler API > also makes it easy for the Xilinx tools to perform > optimization/mapping because AXI-Stream handshaking is not in the way. > Don't get me wrong, AXI-Stream is great, we are just changing the > granularity at which we implement it. > Why start with this block instead of just blocks going forward? I saw the addition of the biquad - great! But, ideally, you'd fix the bug in the delay line and move on if that's what drove people looking into the behavior to begin with? I prefer less AXI streaming interfaces as well, but it just seems like a poor idea to rewrite an already written block and change something out from underneath everyone. > > 2) Why wasn't this re-written block added as a second option for a > vectored > > IIR implementation? There are 64-bits worth of RFNoC block ID's out > there. > > I understand if you don't want to support specific RFNoC blocks anymore, > and > > want to move onto new ones, but can you retire them in a better, more > > thought out way? Moving them to a deprecated folder is fine and giving > one > > or two releases to get people to move away is fine, but please stop > gutting > > and re-writing something like it hasn't completely changed. There are > > people who may want to stick with the old way of doing things (the old > > DDC/DUCs are another recent re-write that could have been handled > better). > > Please give us a chance at trying to maintain stability. > > > > Other than fixing the delay line issue, we did not change the behavior > of the block. We also did not change the software or FPGA interface to > the block. All that changed was the implementation, and that does not > warrant a new block or a deprecated copy. You can argue that if > something/anything changes in a piece of functional code, that it is > inherently unstable. However, that applies to everything from a one > liner bugfix to complete rewrite. That said, if you noticed a bug in > the code, then we will absolutely need to and will fix it. > > In general, RFNoC is still evolving and we are slowly cleaning up > interfaces to reach a point where we are confident that it is easy to > use and it efficiently supports what our customers want to do. We > always try to balance improvements with backwards compatibility, but > there are cases where we cannot guarantee compatibility. In those > cases we will try to minimize the incompatible changes, and increment > the compatibility number to ensure that changes don't go unnoticed to > software. None of that was true for this particular block change. > No offense, but you're wrong in many ways. From an implementation standpoint, your re-write isn't a drop-in replacement. It requires code changes for anyone utilizing the vector_iir module. On top of that, if bugs were found in new implementations, but not in old ones, it's not easy to roll back the changes. Take the DDC/DUC changes for example. I understand why they were done. I understand they are more efficient. If I wanted to use the old verion of the DDC/DUC in a design, it is very difficult to bring that back - even to just do an A/B comparison in case there is a bug. The old code worked fine. It was not broken or riddled with bugs, but for some reason Ettus decided to just ditch it all. Another example is the noc_shell compatibility jump from 4 to 5 changing the RB_FIFOSIZE to be bytes instead of log2(bytes): https://github.com/EttusResearch/fpga/commit/32a0b8985ae9ab636957321c5b51e7d55cf30bb7 I understand the flow control is now byte based in FPGA's master, but if it's the case that the buffering doesn't need to be a power of 2, then why wasn't STR_SINK_FIFOSIZE changed as well during this modification? Moreover, if this is the only real change for the noc_shell, can't the host understand version 4 or 5 of the noc_shell and handle the readback pretty easily? Why did this need to be deprecated and thrown out on the host side code? Lastly, if this FPGA code, being in it's own repository, has a corresponding commit or set of commits associated with the uhd repository, can you please have one, the other, or both reference the changed commits in the other repositories? You have an interest in RFNoC from the community, but you're making it very difficult to embrace due to changes that are not conveyed very well and seemingly random changes that cause problems finding and maintaining stability. It makes code bisection on the host side pretty much impossible to do when trying to find newly introduced bugs. I'll ask once more, though I think it'll fall on deaf ears: please consider keeping old code around a little bit longer, and phasing things out to allow people to get their footing as you make your design changes with RFNoC. Brian
_______________________________________________ USRP-users mailing list USRP-users@lists.ettus.com http://lists.ettus.com/mailman/listinfo/usrp-users_lists.ettus.com
