On Fri, 2009-05-08 at 13:49 +0200, Magnus Lundin wrote: > I'll give it a go, and I do think I understand what is going on under > the hood in some detail.
Thank you for providing your explanations. This has been the most constructive attempt to address the technical issues, and I appreciate your focusing on it from this perspective. > Also there are a lot of cleanup to be done. > > And the documentation about some finer technical points is somewhere > between bad, nonexsistant or can be found in Dominics thesis paper. Which was last updated when? I will make it a priority to grok this document and regurgitate anything useful that I find to the list, but I just want to point out that the lack of up-to-date backing documentation raises questions about the objectivity of anyone's current opinions. I believe internals should be fair game for being rewritten at any time. > Zach Welch wrote: [snip] > > * Are you _certain_ the current bugs being experienced cannot be the > > result of other bugs that were uncovered by the removal of the > > in_handler code? If they are, Øyvind just did us a service. > > > The bugs I have seen have all been caused by pushing unteted code into > hede. Basically to quick and not careful enough. There were some ideas > but this was not a carefull thought out architectural resturcturing. I > hope to show that the old code is definetly a carefully planned archtecture. > > The architecture he proposes will work if cleanly implementd. You are saying his ideas can be implemented successfully? Okay... the only problem here is that you introduced ambiguity in the pronoun 'he'. I can read your reply such that it refers to either Dominic or Øyvind; your intention is not perfectly clear from the context. Assuming that you meant Dominic, your statement's validity requires first assuming that he knows more about the current implementation and the future needs of its architecture than those individuals contributing to the code and working to improve it daily.. Since January, I have seen exactly two posts from him, so he is clearly not an active contributor. Further, I believe all but one of the items listed as "Future Work" in his thesis have been completed, so it seems reasonable to assume that the status quo might have changed. So assuming that I read the 'he' correctly, I believe these assumptions necessarily imbue your statement with a degree of uncertainty. More directly, "X said it is good" holds no water if X is not contributing actively. Øyvind definitely deserves benefit of the doubt given his familiarity and perspective with the code. However, it may turn out that Dominic's idea was the best one and we end up back with it. At this point, I do not believe any of us knows with certainty. > > * Are you _positive_ the "lost performance" will not be regained by any > > other means? If it can, then that argument is insufficient. > > > It might be regained, but in that case I am positive that we we have > either built the same asych callback functionality somwhere else, or > trimmed the USB layer, in which case even more performance can be gained > by reverting. I expect the functionality to be re-implemented in a way that the community will support, even if that means adding back nearly identical code. If I were in Øyvind's shoes, I would rather see the in_handler functionality re-implemented cleanly than just put it back as it was. [snip] > > * How _exactly_ does the in_handler callback fit into it? Again, this > > needs to be explained in fairly complete technical detail. Or maybe > > someone can link to the on-line documentation.... > > > Actually we are talking about two different functionalities here. > > - The handling of asychronous reads from the jtag, and the extra data > treansformation done by the in_handler funtions. > > In order to issue several read requests where the result should be > stored in different locations and avoid doing a full roundtrip for each > call, the address of the receiving location is stored in the in_variable > field when issuing a scan request to the jtag layer. When a > jtag_execute_que() is completed the all oustanding such queued reads are > completed. > So the calling sequence is > - issue as many in scans as possible, > - when we need the return walue, call jtag_execute_queue(), if this > succeds we are guaranteed that the values are in place. I follow you so far. We want to decouple reads of results from the writes, so both can happen asynchronously. jtag_execute_queue() is a sync point that ensures all queued write/read pairs have completed. > This introduces the potential of receiving value going out of scope, > this happens if we ask for a value to be stored in a local variable and > exit before calling jtag_execute_scan, which is a type of an "unused > variable assignment" I am okay up until ", which is...". The last phrase is confusing. > The natural place for this functionality is in the jtag layer. This is not entire clear just yet, but I will assume this for now since you seem to be mostly on-track so far. > The in_handler functionality tells the jtag layer to apply an in_handler > supplied with the inscan command to the received value before storing > it in the receiving location. This transformation is something that must > be done somewhere to adaptet the received jtag data to the data format > of the receiving variable. So some part of the code must handle this > transformation. And it must be done after receiving the jtag in data and > before the result is used. Possible choises are jtag or target variant > specific code. Right: in_handler is/was a scan-specific "value fix-up" callback. > Prime example of this is indata from ARM7 scanchain 1. This data is > bitswapped. Something users that target read register funtions are not > aware of. So the issuer of the Scanchain 1 scan structure tells the > jtag layer about this by adding the relevant function to the in_handler > field. When the data is received the code has long since move away form > this function and we are in the upper layer function that simply > expects a u32 value. Okay. This upper layer that expects the u32... how do the functions that use this value and the functions that setup the scan to retrieve it relate to each other? Specifically, are those functions that setup scans and those that use the results tightly coupled together? To start, I will assume they are. If this is the case, the situation with in_handler seemed to be this: * We want a u32, so setup a scan to get it, with an in_handler to fix the data when it comes in. * At some point, jtag_execute_queue() will retrieve this value and call the in_handler to convert it. * Later yet, the function that needs that value uses it. Likewise, the new implementation may simply look like: * We want a u32, so setup a scan to get it. * At some point, jtag_execute_queue() will retrieve this value. * Layer yet, the function that needs that value can ask to retrieve it. * A helper function takes the former in_handler and lazily converts it. Now, one might assume the opposite: that the scan setup/usage functions are not as tightly coupled. If that is the case, then your very example seems to be a layering violation: you want target-specific fix-ups to be processed by the jtag layer? Am I thoroughly confused here, or is this a correct interpretation of the situation? > > * Where are the patches to finish implementing this work? My impression > > has been that this feature is mostly "for future use", and that is not a > > sufficient reason to keep it -- unless it is being actively worked on. > > > > > The stuff above is finished, it has been used for a long time, cleanup > and polishing is always good but this is a core functionlity that is > working. Right. I do agree that future changes of this magnitude should be completed in advance of being committed to the head of the trunk. I think r1607 should have been floated for at least 24 hours before further work was done, and Øyvind might have been wise to stop until the community was satisfied with the path forward. I apologize to everyone that finds this difficult, but breakage _should_ occasionally happen in the trunk or we are being too conservative in our development practices. [[Not this much breakage though!]] Still, it helps to have clear plans in the public view for a decent amount of time for consideration, then one can just say "I tried to warn you". I have a series of big patches pending, but they should not break anything for anyone. Even in my completely non-functional changes, there will be room for controversy though, so stay tuned. :) > > Show me the "lost work" by answering the above, and I will defend you. > > If this is not forthcoming, then it is in our interest to continue on > > the current path, despite any further objections. > > > > > Hope this explains the background. It does provide more context, but I do not think that it explains why the implementation cannot be improved. The resistance to change in this community worries me. The code is not in _any_ shape to be "preserved" in its current state (unless we are talking about archiving backups). OpenOCD needs to grow and evolve or it will not survive. If change is painful, then evolution is an outright killer. C'est la vie, Zach _______________________________________________ Openocd-development mailing list [email protected] https://lists.berlios.de/mailman/listinfo/openocd-development
