search

jflyper / cleanflight

Clean-code version of the baseflight flight controller firmware
http://cleanflight.com
GNU General Public License v3.0
0 stars 1 forks source link

Abstracted displayPort from CMS #2

Closed martinbudden closed 7 years ago

martinbudden commented 7 years ago

@jflyper , this PR continues the work of abstracting the display device. I've done a number of things:

  1. Made the abstract display a device driver, see drivers/display.c.
  2. Split out the OSD/MAX7456 displayPort out of the OSD code, see `io/osd_max7456.c
  3. Made some minor changes to get OMNIBUSF4 to build.

Note that these changes are a step towards how things should be, not a final result. So there are some compromises made for expediency which need to be sorted out in due course - in particular the externs used in osd_max7456.c. Anything used by a displayPort should be part of the displayPort struct, so that definitely needs fixing.

Note there are two separate abstraction here - the display and the CMS. The low level display primitives are not part of the CMS. However I don't think the split is quite right yet, in particular I think the begin/end/heartbeat functions need further thought.

Your splitting the CMS and the actual concrete menus is also a welcome improvement.

So, this is one more step along the way, and we will gradually lick this into shape.

martinbudden commented 7 years ago

So I've had some further thoughts. begin and end should be part of the displayPort abstraction, but they should be called open and close. It's funny how a simple name change can obscure or clarify what is actually going on. I was slightly blinded to the purpose of the begin and end functions simply because of their names.

jflyper commented 7 years ago

@martinbudden I like it. I was recognizing the display layer as cmsScreen layer, but didn't think of separating it into an abstraction with a separate file. I'm not sure if the display.c should go into drivers; if we call it a pseudo-device, then it will go into drivers, or if we call it an abstract i/o, then it will go into io. Nevertheless, I will merge it and start moving display states into displayPort_t.

As for the begin and end, it was named from how canvas is implemented in the MWOSD code, to begin the canvas mode and to end. But as you pointed out, they are clearly open and close from the perspective of the cms.

martinbudden commented 7 years ago

@jflyper , yes, I'm not 100% sure that display.c should go into drivers, io might be a better home for it. But let's leave it there for the moment, and we can revisit this later.

I've also revised my thoughts one open and close. I now think they should be called grab and release, since what is happening is that the CMS code is grabbing the displayport of another device for its own use. But don't change the name again - my thoughts on this may change again.

One other name that I think is unintuitive is canvas. A canvas is a generic object that you can draw or paint on, whereas here the canvas is a displayport over an external MSP connected OSD. But I wouldn't change the name yet, we can revisit names once the overall structure is more finalised.