Block cursor modification in CScreenDevice

[jnorrid]

How might one go about adding an option for a BLOCK cursor instead of the default underscore cursor for use with text on the screen? I have been looking at line 1000 of /lib/screen.cpp and wondering if I should just change this method or if there is a simpler option elsewhere in the framework. Also found several references to GetUnderline in CharGenerator, but I don't think that is where the cursor is defined. Happy to fork and issue a PR if it just needs to be implemented...

[rsta2]

Yes, CScreenDevice::InvertCursor() is the right method for that purpose. It is called, when the cursor is set and again, when it is removed. CCharGenerator::GetUnderline() returns the line number, where the underline cursor starts. I think, a block cursor should be drawn from line m_nCursorX to m_nCursorX + CCharGenerator::GetUnderline() - 1. If you want to implement a block cursor as an option (e.g. SCREEN_BLOCK_CURSOR) and send a PR, please do.

[jnorrid]

Aren't the "lines" on the y axis? I thought m_nCursorX specifies a location along the width.

[rsta2]

Yes, of course it's m_nCursorY. Sorry.

[jnorrid]

No problem - thank you for your guidance. I have been doing some prototyping with a modified version of CScreenDevice::InvertCursor(). When I modify anything in that routine, I get the following error message at startup on a Pi4:

heapallocator.cpp(191): assertion failed: pBlockHeader->nMagic == HEAP_BLOCK_MAGIC

I am using the 08 sample to try this out and the ONLY mods I have attempted are in the screen.cpp file and I don't get any warnings or errors from GCC when running makeall.

Any idea why -ANY- modification of InvertCursor() would blow up like this?

[rsta2]

For the block cursor you probably need to save more pixels in m_pCursorPixels. I can only guess, that you have still the allocation for the underline cursor here? The underline cursor needs 48 bytes for 3 lines there, a block cursor with 16 lines occupies 256 bytes (with 8 pixels per line and 16 bits per pixel). With the old allocation this would overwrite the header of the following memory block in the heap, and when it is freed, you will get this message.

[jnorrid]

OK - I missed where it was being setup on line 127 of screen.cpp in the Initialize method - thank you for pointing this out for me! I will probably work on this a bit more this week as I will have some cycles. I did get it to work in a hard coded way, so I know it is possible, just need to spend a bit more time crafting it so it fits in with the rest of the framework. BTW - thank you also for working on this project - it is super cool to be able to do bare-metal but to have a bit easier access to some of the trickier parts with the abstractions delivered here in the C++ framework. It's really great work and very useful for those trying to learn how to do this!

[rsta2]

Take the time you need. Thanks for appreciating Circle!

[rsta2]

Have you made progress here?

[jnorrid]

Yes - got it to work, although I am not sure if it meets your standards. While I am able to get it working, I run my systems on a KVM and if I change to a different machine using the KVM and then back, the system halts with:

Transfer error 36 on endpoint 3 followed by "Trying to allocate more than 0 instances."

All work has been committed to a fork here: https://github.com/jnorrid/circle. The vast majority of the changes are to screen.h, screen.cpp, and sample 08. I am using makeall in the repo root to build changes to the library, then using make and then make flash to send the sample08 code updates to the serial bootloader.

I have not yet issued PR as I am not confident that I have done this correctly but I am stuck as to why it is failing after KVM position change and revert.

[rsta2]

There was still a problem in the memory allocation, which was leading to the destruction of the data of a class allocator (please see the commit). This caused the panic message. I fixed it. Everything is now on the develop branch in the Circle repo. The block cursor can be tested with test/screen-ansi-colors too. Thank you for this contribution!

BTW. I never tested with KVM. Transfer error 36 is a USB split-transaction error, which probably happens, when you switch the USB.

[jnorrid]

I see. Well, I suppose I should have properly read the contribution guidelines and cloned DEVELOP instead of MASTER, then gone ahead and generated a PR. Oh well, guess I won't be listed as a contributor since you went ahead and just integrated it :(

FYI - the Transfer Error 36 DOES NOT occur with the standard UNDERLINE cursor in a vanilla clone of the MASTER repo. It only happened with the code I had built and executed with the method call in sample-08 when changing the KVM switch to another position and then back. Worked fine with step47 MASTER and no changes on KVM.

[rsta2]

My apologies, I was probably too quick here. I thought, it's enough to be the author of a commit in the repo to be listed as a contributor, but it's obviously not. It must be a PR. But yes, you would not be able to use your master branch for creating a PR for Circle, because there are commits on this branch, which do not belong into the repo. But of course, you would have been able to create a new branch and cherry-pick the needed commits there, and create the PR with it.

I often had to merge contributions for Circle manually, because often PRs were not for the develop branch, which is required. I think, Circle is not big enough as a project, to enforce a process for contributions, which would mean to reject PRs, which are not according to the guidelines. So I merged these PRs manually. But I did not gave you the opportunity, to create a PR. :(

Unfortunately I cannot find a modifications after the release Step47, which may affect the Transfer error 36. There is a chance, that the overwritten memory from the CScreenDevice class, was causing this. I don't know, if the problem still occurs with the latest version from the develop branch?

[jnorrid]

OK - I performed a fresh git clone and changed to the develop branch locally to pick up the commit. I then added my custom Config.mk and Rules.mk modification to point to my local toolchain and to setup use of the serial bootloader. I uncommented line 101 in the kernel.cpp file from sample-08-usbkeyboard. I then performed makeall in local repo root followed by make, and make flash to push the code to one of my Pi4 machines hooked to the KVM. Everything works fine now when switching KVM positions between different machines and back to the Pi, so it appears that the issue I was encountering did, in fact, have to do with the memory allocation problem you mentioned you had fixed in develop. Looks like the change is good to go now.

[rsta2]

Good to know. Thanks for testing this again!