Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMS for HoTT-Textmode. #6224

Merged
merged 4 commits into from Aug 16, 2018

Conversation

Projects
None yet
8 participants
@Scavanger
Copy link
Contributor

Scavanger commented Jun 26, 2018

After the discussion here #6102, @Ziege-One and me decided to bring the CMS to the Graupner transmitters. It can controlled over the touch-pad or over sticks.
To avoid errors on F3 targets, we limit Textmode to F4 and F7 boards.
A short Video:
https://www.youtube.com/watch?v=gSDDW5ZFEv4
hott-cms-main
hott-cms-info
hott-cms-imu

holdCount = 1;
repeatCount = 1;
repeatBase = 0;
if (externKey != CMS_KEY_NONE) {

This comment has been minimized.

@ledvinap

ledvinap Jun 26, 2018

Contributor

IMO current stick-key handling code should be separated into own function(s) (cmsProcessStickKey? cmsUpdateStickKey? )
and then

  if (externKey != CMS_KEY_NONE) {
    delay = cmsHandleKey(pCurrentDisplay, externKey);
    cmsProcessStickKeyReset(delay);
    externKey = CMS_KEY_NONE;
  } else {
   cmsProcessStickKey();
  }

current function is getting too complicated (it would be ideal to separate stick-key mapping from repeat)

This comment has been minimized.

@Scavanger

Scavanger Jun 26, 2018

Author Contributor

Yes, but what should a cmsProcessStickKeyReset do?

This comment has been minimized.

@ledvinap

ledvinap Jun 27, 2018

Contributor

it should probably be cmsProcessStickKeyStop - stop autorepeat, do nothing until stick position is CMS_KEY_NONE.
And set rcDelayMs if sticks are centered ...

This comment has been minimized.

@Scavanger

Scavanger Jun 27, 2018

Author Contributor

I agree, but I think this has more to do with refactoring the CMS, and not with HoTT Textmode.

This comment has been minimized.

@ledvinap

ledvinap Jun 27, 2018

Contributor

Yes, but these problems were quite localized and hidden until this PR ...
The code was far from ideal, this PR is exposing its design flaws

@@ -24,6 +24,14 @@

#include "common/time.h"

#define CMS_KEY_NONE 0

This comment has been minimized.

@ledvinap

ledvinap Jun 26, 2018

Contributor

enum will be cleaner

This comment has been minimized.

@Scavanger

Scavanger Jun 27, 2018

Author Contributor

Same here, perhaps we should refactor the CMS.

This comment has been minimized.

@ledvinap

ledvinap Jun 27, 2018

Contributor

The constants are public now ...

This comment has been minimized.

@Scavanger

Scavanger Jun 27, 2018

Author Contributor

OK, convinced.



// Correct fake display dimensions
if (row <= ROW_CORRECTION - 1)

This comment has been minimized.

@ledvinap

ledvinap Jun 26, 2018

Contributor

row < ROW_CORRECTION ?



// Correct fake display dimensions
if (row <= ROW_CORRECTION - 1)

This comment has been minimized.

@ledvinap

ledvinap Jun 26, 2018

Contributor

Also :

if (row < ROW_CORRECTION || row >= HOTT_TEXTMODE_DISPLAY_ROWS + ROW_CORRECTION 
    || col < COL_CORRECTION || col >= HOTT_TEXTMODE_DISPLAY_COLUMNS + COL_CORRECTION) {
   return 0;
}
row -= ROW_CORRECTION;
col -= COL_CORRECTION;
{
for (int row = 0; row < displayPort->rows; row++) {
for (int col= 0; col < displayPort->cols; col++) {
hottWriteChar(displayPort, col, row, ' ');

This comment has been minimized.

@ledvinap

ledvinap Jun 26, 2018

Contributor

(this is clearing invisible row/cols too)

This comment has been minimized.

@Scavanger

Scavanger Jun 26, 2018

Author Contributor

No, hottWriteChar(...) is in displayport_hott.c and checks for invisible row/cols.
You mean hottTextmodeWriteChar(...) in hott.c

This comment has been minimized.

@ledvinap

ledvinap Jun 27, 2018

Contributor

displayPort->rows/cols is larger than actual hottTextmodeWriteChar accepted area, so some writes are skipped ... it is only minor detail ...

case HOTTV4_BUTTON_PREV:
cmsSetExternKey(CMS_KEY_LEFT);
if (esc)
cmsMenuOpen();

This comment has been minimized.

@ledvinap

ledvinap Jun 26, 2018

Contributor

there are 2 way to enter menu - sticks and key, kut key processing is mixed together... this makes code harder to read/maintain ...

This comment has been minimized.

@Scavanger

Scavanger Jun 26, 2018

Author Contributor

Not really, yes you can select the HoTT CMS menu by sticks, but only when you access the cms by key its get visible. This code makes sure the CMS is open until textmode is closed.
Perhaps the argument name should be keepCmsOpen.

cfTaskInfo_t taskInfo;
getTaskInfo(TASK_TELEMETRY, &taskInfo);
telemetryPeriod = taskInfo.desiredPeriod;
rescheduleTask(TASK_TELEMETRY, TASK_PERIOD_HZ(1000));

This comment has been minimized.

@ledvinap

ledvinap Jun 26, 2018

Contributor

maybe just use macro to define task rate (as for HOTT_TEXTMODE_RX_SCHEDULE)

This comment has been minimized.

@Scavanger

Scavanger Jun 26, 2018

Author Contributor

Oops, overlooked.

This comment has been minimized.

@ledvinap

ledvinap Jun 27, 2018

Contributor

Sorry, bad wording.
do not use telemetryPeriod, but use hardcoded values HOTT_TEXTMODE_RX_SCHEDULE / HOTT_RX_SCHEDULE

@mikeller mikeller added this to the Betaflight v3.4 milestone Jun 27, 2018

@mikeller
Copy link
Member

mikeller left a comment

Good work, guys. Only a couple of style problems to fix.



// Correct fake display dimensions
if (row <= ROW_CORRECTION - 1)

This comment has been minimized.

@mikeller

mikeller Jun 27, 2018

Member

Please always use blocks with if, and for, and while.

uint8_t fill1; //#02 constant value 0x00
uint8_t warning_beeps;//#03 1=A 2=B ...
uint8_t msg_txt[HOTT_TEXTMODE_MSG_TEXT_LEN]; //#04 ASCII text to display to
typedef struct HOTT_TEXTMODE_MSG_s {

This comment has been minimized.

@mikeller

mikeller Jun 27, 2018

Member

Should be hottTextModeMsg_s, hottTextModeMsg_t.

This comment has been minimized.

@Scavanger

Scavanger Jun 27, 2018

Author Contributor

Yes! Perhaps someone should rename the other structs in hott.c, too ;-)

This comment has been minimized.

@mikeller

mikeller Jun 28, 2018

Member

One of the principles I find to be very beneficial to long term code maintainability and quality is 'leave everything at least as good as you found it'. If you modify something that could be better - improve it. If you add something new, do it right. Cleanup can (and probably should be) done in a separate pull request, and I understand if it isn't done, because we all do this for a hobby.

static bool setEscBack = false;

if (!textmodeIsAlive)
{

This comment has been minimized.

@mikeller

mikeller Jun 27, 2018

Member

Please use K&R style (braces on new line only for function definitions, on the same line otherwise).

@@ -495,6 +604,11 @@ static void hottCheckSerialData(uint32_t currentMicros)
*/
processBinaryModeRequest(address);
}
#if defined (USE_HOTT_TEXTMODE) && defined (USE_CMS)
else if (requestId == HOTTV4_TEXT_MODE_REQUEST_ID){

This comment has been minimized.

@mikeller

mikeller Jun 27, 2018

Member

Space missing.

{
if (externKey == CMS_KEY_NONE)
externKey = extKey;
}

This comment has been minimized.

@McGiverGim

McGiverGim Jun 27, 2018

Member

Always use { } for if, while, etc. it has easy maintenance in the future.

@Scavanger Scavanger force-pushed the Scavanger:CMS-for-Hott-Textmode branch from 9dda86e to a63beb5 Jun 30, 2018

@hydra

This comment has been minimized.

Copy link
Contributor

hydra commented Jul 5, 2018

@@ -607,6 +608,10 @@ void init(void)
cmsDisplayPortRegister(displayPortCrsfInit());
#endif

#if defined (USE_HOTT_TEXTMODE) && defined (USE_CMS)
cmsDisplayPortRegister(displayPortHottInit());

This comment has been minimized.

@codecae

codecae Jul 5, 2018

Member

Might want to register this outside of fc/fc_init.c and instead in the telemetry subsystem. Don't want to register multiple display ports and cause unwanted cycling between them when people aren't using the protocol or have telemetry enabled.

This comment has been minimized.

@codecae

codecae Jul 5, 2018

Member

I am doing the same for CMS over CRSF.

@Scavanger

This comment has been minimized.

Copy link
Contributor Author

Scavanger commented Jul 5, 2018

@mikeller
This PR was "Ready to merge" and now "Review required".
Whats wrong?

@codecae

This comment has been minimized.

Copy link
Member

codecae commented Jul 5, 2018

@Scavanger you pushed commit a63beb5 - this dismissed previous approvals.

@Pierre-A

This comment has been minimized.

Copy link
Contributor

Pierre-A commented Jul 5, 2018

Been a long time.. congrats for this nice job to you all 👍 !

@mikeller
Copy link
Member

mikeller left a comment

See comment from @codecae re cmsDisplayPortRegister(). This has broken us already for CRSF (#6286), so we should fix it here before this happens.

@@ -141,6 +141,11 @@ displayPort_t *displayPortHottInit()
return &hottDisplayPort;
}

void hottDisplayportRegister()
{
cmsDisplayPortRegister(displayPortHottInit());

This comment has been minimized.

@codecae

codecae Jul 6, 2018

Member

Not sure this needs to be exported as part of displayport_hott .. just adding cmsDisplayPortRegister(displayPortHottInit()); to the telemetry initialization should be sufficient, as displayPortHotInit is already exported.

This comment has been minimized.

@Scavanger

Scavanger Jul 6, 2018

Author Contributor

I don't want a CMS dependency in hott.c, because the "display" shouldn't know CMS and CMS shouldn't know its displays directly. Both should only know displayport.
Displayport is the interface to CMS, so all input/output goes through it.

This comment has been minimized.

@codecae

codecae Jul 6, 2018

Member

I guess it's just a matter of philosophy. To me, it seems that before the registration burden was carried by fc and it is now being transferred to the telemetry system. The init function is implemented to return a pointer so the implementing system can obtain a reference to the displayport and register it with the producer (cms). If the displayport is to be tightly coupled with CMS and telemetry, there is no reason to export the init function and it should be declared statically.

This comment has been minimized.

@codecae

codecae Jul 6, 2018

Member

Just my opinion so take it as you'd like :) I'm not here to judge or debate your philosophy. Just sharing my thoughts.

@@ -342,6 +342,10 @@ void initHoTTTelemetry(void)
portConfig = findSerialPortConfig(FUNCTION_TELEMETRY_HOTT);
hottPortSharing = determinePortSharing(portConfig, FUNCTION_TELEMETRY_HOTT);

#if defined (USE_HOTT_TEXTMODE) && defined (USE_CMS)
hottDisplayportRegister();

This comment has been minimized.

@codecae

codecae Jul 6, 2018

Member

cmsDisplayPortRegister(displayPortHottInit());

@hydra

This comment has been minimized.

Copy link
Contributor

hydra commented Jul 7, 2018

merge it, merge it, merge it!

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Jul 7, 2018

@hydra: See
image

@hydra

This comment has been minimized.

Copy link
Contributor

hydra commented Jul 8, 2018

@mikeller ahh ok, github doesnt show milestone in mobile web view...

@IvaAlex

This comment has been minimized.

Copy link

IvaAlex commented Jul 13, 2018

Guys, can you please tell me why TEXTMODE is limited to F4 and F7 targets only ? why not F3 ? flash size ?

@Scavanger

This comment has been minimized.

Copy link
Contributor Author

Scavanger commented Jul 13, 2018

@IvaAlex; Yes!
You can try to add #define USE_HOTT_TEXTMODE to your target.h and give it a try. On some F3 targets is enough free flash.

@IvaAlex

This comment has been minimized.

Copy link

IvaAlex commented Jul 17, 2018

20180716_203854
Thank you guys for developing ! Tested on Betaflight-F3, mz-18 and gr12L. Had to take out Spektrum code because lack of space on the controller.

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Jul 17, 2018

Wow, this looks shiny! :-D

@mikeller mikeller merged commit fdb4e20 into betaflight:master Aug 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.