Add support for 8 & 16 bits sram traces and SH1106 OLED display #533
Conversation
This enables tracing of 16 bits IO registers, e.g. SP (SPH+SPL) as well as FW variables, e.g. heap (__brkval)
+ associated example/demo code
Needles array was initialied with 0s (end of string) preventing display of characters behind
encoder_phase was operating on 4 bits but it should be 6 to offer 64 states - 16 needles (Belt Phase "period") - 4 v1/v2 states for each needle KH270 didn't took into account that: - the first needle was not mapped to solenoid 0 (but 4 when moving to the right) - when moving to the left, solenoid to set has to be shifted by 6
|
I have pulled this into a local branch and built it to see if it is ready for merging. I think there is probably some good stuff here, but unfortunately, it is not ready to merge. There are several problems: There is too much here. An additional example board (board_ayab) is added that is not mentioned in the description! After finding that, I did not dig deeper, so there may be more. For me this is a perfect example of why it is poor practice to present your "master" branch in a PR. Ideally, a PR should have its own branch containing selected commits that add a single feature or fix. So there should be two or three PRs using this material. The new examples do not conform to the standard pattern. For example, after building from the top-level, this works: Most of the others can be run the same way and can conveniently be run as a group (from a script) as an additional regression test. Yours could work the same way, but do not. Indeed, even the given example command does not work: The firmware is not built because it does not follow the naming convention. The README.md files look good on github but ugly at the point of use. Is the user expected to run a dedicated Markdown viewer? Plain text please! Some inconsistent code formatting is present. The project convention is to use tabs to represent a four-character indent and @buserror has previously been quite strict about that. |
|
Thanks for you review.
=> What should I do:
|
|
Splitting into two PRs does seem the way to go. I have dug into the code a little more and have some questions and comments on the VCD enhancement. First, why is there a call to _call_sram_irqs() in avr_core_watch_read() (sim_core.c)? I think that puts a redundant entry into the VCD file each time a traced variable is read. Second, the change might be considered incomplete, as the new VCD traces may be requested in the firmware, or by a command option in the example, but not by a the run_avr command. And the core of the enhancement looks like a useful general-purpose feature of the simulator, similar to and partially overlapping the current feature of IRQs called on I/O register access. There could be new functions to use it similar to avr_register_io_read() and avr_register_io_write(). Finally, the array size (16) should have #define as it is used on two places. or use ARRAY_SIZE. On the other part, sh1106, I would not like to lose the example program. It looks quite close to a test harness for a complete application. That is more than we see in other example programs; they look like demos. But a casual user should be able to run it without extra steps. A way to do that may be to include the firmware in compiled form. I think binaries in a Git repository are considered to be red flags, or at least bad form, but a base64-encoded version might pass. The makefile could convert it automatically. Also, the README could have a URL for the firmware binary, but that might not last. For now, please post a compiled binary here. I would like to see this run. |
|
OK, I'll create two new PRs; one for SRAM traces, the other for SH1106. Wrt your questions:
Wrt sh1106, I can check in one .elf file. |
Add support for (one commit for each):
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).
SH1106 demo code emulates a complex system (OLED display, joystick for navigation & buttons) integrating support for SRAM as well a Program Counter tracing from the command line.
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