This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/simtrace@lists.osmocom.org/.
Min Xu mxu at sanjole.comHi Harald Thank you for getting back to me so quickly. I am using Emacs on an Ubuntu netbook to do the edits, which likes to reformat sections as I move (copy/paste section around) and I haven't figured out a command to undo those changes. It'd be nice if there is a .emacsrc or something that automatically formats the code to the project standard? (presumably someone has one? ..) I started only wanted to make one change and submit (was initially the req_ctx), but couldn't reliably verify it until the debugp was working better without affecting the system, so the changes ballooned up to this stage. So I wanted to submit the changes before I change the code further. I will try limit the scope of changes in my future updates. To your questions: 1) The numerical values of the RCTX_STATE are changed to facilitate the choosing of the FIFO/double linked list of req_ctx to use. An array of FIFO, using the state to index into the array to get the FIFO is simplest. When I looked how the reqctx_find / ... functions are used, I found all callers use the constant name rather than any specific numeric value. 2) I will look at this tomorrow. But I was under the impression on how the lib_Atmel?? (sorry, can't access the source at the moment) was invoked to install the irq handler, that the entry point to the interrupt would be directly the function passed in. I looked at the sysirq_handler.c?? more closely than the other so I wasn't aware of the Cstartup.c file Since I increased the req_ctx buffer size to 1kb each, most req_ctx that are transferred to pc contain many many apdus but very often, the req_ctx do not begin at the start of an apdu. So I would like to have an offset (2 byte, since it could be > 256) to indicate the position of the first start of an apdu. After thinking about it more, I would also like to add a sequence number into the header (making the header 8 bytes then?) to better allow for detecting dropped req_ctx (if there are any..). The offset would be 8 for a req_ctx that starts with a new APDU, and would be 0 for an req_ctx that only contains a middle or last fragment of an APDU. I have already modified the host software to run on Windows (currently using Windows 7 x64), so I will probably make those changes locally only to help me detect any problem I might still be facing. Again, thank you very much for getting back so quickly, and I appreciate your product very much. Best Regards On Wed, Sep 4, 2013 at 9:31 PM, Harald Welte <laforge at gnumonks.org> wrote: > Dear Min Xu, > > On Wed, Sep 04, 2013 at 07:47:46PM -1000, Min Xu wrote: > > I made a bunch of changes that significantly improved my test scenario: > > I _really_ appreciate your excellent technical work. However, I would > like you to go the little extra effort it takes to 'properly' interact > with community based Free Software projects, where there are rules like > > * only one feature / logical change per patch, resulting in a series > of incremental patches, each taking us one step further, each > rendering a compile-able and functional build after being applied. > * no re-formatting of code (whitespace changes) > * no patch for local changes like different compiler name in Makefile > * stick to coding style of the project (tab-wide indent, ...) for > consistency > * remove old/dead code rather than comment it out (RCTX_STATE_...) > * avoid asm() statements whenever possible. If you need them, please > wrap them in an inline C function with descriptive name. > > Some quesetions: > * why did the numberic values of RCTX_STATE_* have t be changed rather > than amended by your new values, keeping the old ones as-is? > > * interrupt nesting _should_ have been active all the time, see > IRQ_Handler_Entry in Cstartup.S, where we first save SPSR and then > un-set the IRQ and FIQ bits _before_ branching to the interrupt > handler function: > > /*- Enable Interrupt and Switch in Supervisor Mode */ > msr CPSR_c, #ARM_MODE_SVC > > Where ARM_MODE_SVC is set to 0x13, i.e. without 0x40 or 0x80. > > So if there's something wrong with the existing code, it should be > fixed there rather than two copies of inline-asm in the USART and USB > IRQ handler routines. > > So I do want to merge this very much, but I think it needs clean-up > before it can be merged. I don't expect to have much time for this in > the next couple of weeks, so I would be happy if either you or somebody > else on the list could work on this. > > Regarding your comment about changing the USB protocol: This would lead > to host / firmware version incompatibilities, and I'd like to avoid that > if possible in any way. We already have the simtrace_hdr.flags > structure member, which contains things like SIMTRACE_FLAG_ATR. I'm not > sure if I understand your request fully, but why not simply add flags > like: > > * First fragment of a fragmented APDU > * More fragments to follow for this APDU > > This way a new APDU still has to start at the beginning of a USB > transfer, but no changes to the simtrace_hdr are required. > > > Thanks again, > Harald > > > -- > - Harald Welte <laforge at gnumonks.org> > http://laforge.gnumonks.org/ > > ============================================================================ > "Privacy in residential applications is a desirable marketing option." > (ETSI EN 300 175-7 Ch. > A6) > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/simtrace/attachments/20130904/d81debc8/attachment.htm>