Add support for 8 & 16 bits sram traces #540
Conversation
This enables tracing of 16 bits IO registers, e.g. SP (SPH+SPL) as well as FW variables, e.g. heap (__brkval)
|
I am still not convinced the the call added to avr_core_watch_read(). On this, you wrote "call to _call_sram_irqs() in avr_core_watch_read() has been made to intercept (hardware) update from the I/O region as well, i.e. not only SRAM but everything between address 0x020 and RAMEND". Do you have an example? It seems to me that this applies to I/O registers which may (should?) have an IRQ associated already, and that can be used to trigger the VCD output. Also any update made on reading would be recorded at the time of the read, not the write. The resulting VCD file may be very confusing to a trusting user who does not know the mechanism. |
|
For sram8 there is indeed an overlap with existing trace capability but if you want to dump 16b registers in that I/O regions it is required. If I correctly remember, I didn't found a generic way to intercept "write from outside" into hardware registers, e.g. ADC updates and decided to catch these when CPU was reading them but I'm open to better strategies. |
|
I think that 16 bits should make no difference: your code outputs the new value in any case. It may do it twice, but that may reflect how the real device works. The ADC result is a good example. Without a code change, there seems no way to pick up the value when it is written. You might be able to use an interrupt IRQ, except that the value is updated after the interrupt is raised. (That does look wrong.) Register space read/write IRQs only trigger on core access, not peripherals, and that seems wrong also. I think it has been discussed here somewhere before. So the call in avr_core_watch_read() could be seen as working around a bug. Why not fix the bug directly? If the ADC code used avr_core_watch_write() to update the result registers, there would be no problem. It currently writes to the RAM array directly, which might have been considered an optimisation. Now that you have introduced this new form of monitoring, it is a bug! |
|
Agree with you, it should be done within the function updating the register and this is a workaround (I had to stop somewhere :-) |
|
I think there is a danger of introducing infinite loops with a wholsale update. A peripheral may write an I?O register, be called back and so on. There does seem to be an open design issue with making peripheral writes visible to VCD and gdb. My feeling is that they should be, but the required changes may be fairly large. It would be useful to have an opinion from @buserror. For now, I suggest just fixing the ADC (anything else your example needs?) and removing that call and I think this one is good. |
|
OK, I'll work on that (only ADC if I correctly remember), give me a few days. |
|
Back to this one finally ... if I replace the two avr->data updates here below by the corresponding avr_core_watch_write() and remove my call to _call_sram_irqs() in avr_core_watch_read() it should achieve the intended goal ? Wrt timing, it will also be correct as it will take place once adc interrupt fires. https://github.com/buserror/simavr/blob/master/simavr/sim/avr_adc.c#L42 static avr_cycle_count_t
avr_adc_int_raise(
struct avr_t * avr, avr_cycle_count_t when, void * param)
{
avr_adc_t * p = (avr_adc_t *)param;
if (avr_regbit_get(avr, p->aden)) {
// if the interrupts are not used, still raised the UDRE and TXC flag
avr_raise_interrupt(avr, &p->adc);
avr_regbit_clear(avr, p->adsc);
if( p->adts_mode == avr_adts_free_running )
avr_raise_irq(p->io.irq + ADC_IRQ_IN_TRIGGER, 1);
if (!p->read_status) {
/* Update I/O registers. */
//avr->data[p->r_adcl] = p->result & 0xff;
//avr->data[p->r_adch] = p->result >> 8;
avr_core_watch_write(avr, p->r_adcl, p->result & 0xff);
avr_core_watch_write(avr, p->r_adch, p->result >> 8);
}
}
return 0;
}If OK with you I'll modify my code as illustrated above, test and push on my PR branch. |
This was introduced as a workaround to catch IO register updates from hardware for the new sram tracing facility but gatk55 suggested buserror#540 to rather modify peripherals instead. This commit fixes the adc by using calls to avr_core_watch_write intead of direct update of avr->data[] in avr_adc_int_raise().
This was introduced as a workaround to catch IO register updates from hardware for the new sram tracing facility but gatk55 suggested buserror#540 to rather modify peripherals instead. This commit fixes the adc by using calls to avr_core_watch_write intead of direct update of avr->data[] in avr_adc_int_raise().
|
Yes, that is what I was thinking of. It is unrelated, but I think it makes sense to mode the writing of the result registers to the top of the containing block. That way any IRQ handlers for the interrupt raising or re-triggering can see the new value. That makes sense to me. Your choice to incluse it or not. Thanks, G. |
|
Agree, that should have been the case already -> fixed in 71c616d |
Add support for 8 & 16 bits SRAM traces (0x20-ramend, i.e. overlap IO range to handle 16 bits registers)
SRAM tracing allows tracking of 16 bits registers, e.g. SP (Stack Pointer) as well as firmware variables, e.g. __brkval (Heap), that have no fixed adresses (using new AVR_MMCU_TAG_VCD_SRAM_8 and AVR_MMCU_TAG_VCD_SRAM_16 macro to populate the avr_mmcu_vcd_trace_t in the .mmcu section).
This has been developped to analyze heap-stack margin and evolution over time in the following project: https://github.com/jpcornil-git/Cone2Hank.
VCD trace example:
https://github.com/jpcornil-git/simavr/tree/master/examples/board_sh1106#examples