1. Forum
  3. News, Announcements and Feedback
  5. Feedback & Questions
  7. PB, request for code review and merge back(2)

PB, request for code review and merge back(2)

More
16 Nov 2012 17:27 - 16 Nov 2012 17:40 #2948 by sbstnp
Replied by sbstnp on topic PB, request for code review and merge back(2)

PhracturedBlue wrote: I'm still open to being convinced, but so far I have heard nothing to make me think the implementation you have brings any functionality over what I've stated I want.


It's all about user experience. Having fixed menu entries helps newbies and people not familiar with the firmware. Having customization options makes people already at home with Deviation able to make it fit their way of handling things. This is never a BAD THING(tm).

Look, I'm not trying to convince you, but if these options do not affect performance or usability they should be allowed in.

On Devo 10 this firmware will have 3 cutomization options:

1. Main screen
2. Main screen shortcuts to different pages
3. Main menu

Main screen shortcuts will not be that powerful unless we limit the option to about 2 pages left, 2 pages right, otherwise will become obscure and rarely used.

Think about it, you created Deviation for Devo 8 with themes in mind. Devo 10 will not have such things. Having menu customization is a plus for such a limited user experience.

EDIT:

I forgot to ask you if you tested the changes, everything I've written above was based on the assumption you did. If not, suvsuv did the following things:

1. Fixed Main Menu to Model setup and Transmitter configuration
2. Implemented a way to add more menu items.

That's it, you can't remove or change order of the first 2 menu entries, and that's good because you can't screw up documentation. You can only add.
Devo 10 + 4in1
Spektrum Dx9
FrSky Taranis + TBS Crossfire
Last edit: 16 Nov 2012 17:40 by sbstnp.

Please Log in or Create an account to join the conversation.

More
16 Nov 2012 17:40 #2949 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
but really, what do you plan to put in the main menu? Having too many configuration options absolutely can be a bad thing if they don't bring real value. It makes the system harder to learn, and harder to share. I'm all for adding configuration when it brings value, but I just can't see it here.

So far everything that I can see going in the main menu either:
a) won't be accessed often enough that it causes any hardship for it to be in the 2nd level menu
or
b) is accessed so frequently you'd want it available from the main page

Please Log in or Create an account to join the conversation.

More
16 Nov 2012 17:42 - 16 Nov 2012 17:42 #2950 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)

sbstnp wrote: I forgot to ask you if you tested the changes, everything I've written above was based on the assumption you did. If not, suvsuv did the following things:

Of course. it wouldn't really be fair for me to criticize without trying it.
Last edit: 16 Nov 2012 17:42 by PhracturedBlue.

Please Log in or Create an account to join the conversation.

More
16 Nov 2012 17:42 #2951 by sbstnp
Replied by sbstnp on topic PB, request for code review and merge back(2)

PhracturedBlue wrote: but really, what do you plan to put in the main menu? Having too many configuration options absolutely can be a bad thing if they don't bring real value. It makes the system harder to learn, and harder to share. I'm all for adding configuration when it brings value, but I just can't see it here.

So far everything that I can see going in the main menu either:
a) won't be accessed often enough that it causes any hardship for it to be in the 2nd level menu
or
b) is accessed so frequently you'd want it available from the main page


Edited my post above while you were posting.

Example:

I added USB and Mixer since I'm playing alot with model setup currently. When my model configurations will become stable, I'll add something else. What else? No idea yet, but having the power to do so makes me feel good :)
Devo 10 + 4in1
Spektrum Dx9
FrSky Taranis + TBS Crossfire

Please Log in or Create an account to join the conversation.

More
16 Nov 2012 17:45 #2952 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)

sbstnp wrote: I added USB and Mixer since I'm playing alot with model setup currently. When my model configurations will become stable, I'll add something else. What else? No idea yet, but having the power to do so makes me feel good :)

USB should just be on the main page.
Mixer is actually faster without being there (2 presses of enter vs enter, up/down, enter)

I can't help that it makes you feel better, but you wouldn't have felt any worse had it never been there in the 1st place

Please Log in or Create an account to join the conversation.

More
16 Nov 2012 17:52 - 16 Nov 2012 17:58 #2953 by sbstnp
Replied by sbstnp on topic PB, request for code review and merge back(2)

PhracturedBlue wrote:

sbstnp wrote: I added USB and Mixer since I'm playing alot with model setup currently. When my model configurations will become stable, I'll add something else. What else? No idea yet, but having the power to do so makes me feel good :)

USB should just be on the main page.
Mixer is actually faster without being there (2 presses of enter vs enter, up/down, enter)

I can't help that it makes you feel better, but you wouldn't have felt any worse had it never been there in the 1st place


No not really, I use Mixer straight after USB, so it's faster (ext/down/enter vs ext/up/up/enter/enter). See, something you haven't taken into account. Just give it some thought, it will make sense.


A nice read about UX below:
52weeksofux.com/post/475093254/10-principles-of-ux
Devo 10 + 4in1
Spektrum Dx9
FrSky Taranis + TBS Crossfire
Last edit: 16 Nov 2012 17:58 by sbstnp.

Please Log in or Create an account to join the conversation.

More
16 Nov 2012 23:53 #2954 by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back(2)
PB, you design this great project and have it shared with others。 Since I love to use it , I join and help you to make it be better. Although I believe you can do everything all by yourself, you still need others' help to test to find bugs that you might not notice, to port to other platform quicker , to and add more features. All others' contribution would help your project be more stable and user-friendly.
For certain areas, people might not be able to make consensus, if it is not critical to platform, e.g., whether to put telemetry monitor to the make menu won't hurt any user-experience at all, give users a customization way will always the best solution. By this way, the mixer/tx config are always in the main menu, and people can have their own DIY menu items based on their judgement.

Please Log in or Create an account to join the conversation.

More
17 Nov 2012 00:37 #2955 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
I have repeatedly stated how grateful I am to you for the work you have done and are doing. That is all true, but it does not mean I want to sacrifice my vision for how Deviation should work.

There are 3 options here:
1) Convince me I'm wrong. All it will take is a compelling argument for why this will significantly help end-users vs the alternatives. So far I haven't seen anything significant enough to sway me.
2) Live with my decision
3) Fork the project and go it your own way.

To me, this whole thing feels like you not liking my decision to move the Telemetry menu and inventing a system to work around that. I've already stated that I don't want the telemetry menu where it is, and that it is a temporary solution. Maybe that was not your intent, but it is how it appears to me.

I am in the middle of implementing my vision of how this should work. If, after its done you still feel there is a significant loss of functionality, then we can discuss this further. For the moment, I've pulled everything I will until I'm done with my code changes.

Please Log in or Create an account to join the conversation.

More
17 Nov 2012 06:02 #2959 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
suvsuv,
I'd like your opinion on how to finish the quick-page functionality. I have it working properly (mostly) for the Devo8, but have run into a couple of issues on the Devo10. I can solve them, but my solution is a bit hacky, and you have a better feeling for the layout.

Basically I ran into 2 problems:
1) I need to identify whether I am in the menu or not so I can properly handle the button-press when on a quick page. For instance from main-page to telemetry works, but I can't get back to the main page because the buttons are intercepted. Also exit goes back to the menu because each page is coded to do so.

2) the telemetry page grabs Left/Right which are my default hotkeys. Since the telemetry page is 2 pages, I should basically be able to assign each of these separately to a quickpage location so I can go main->telem->gps->main

As I said, I can make it work, but may make a mess of the button handler stuff, so I'd like your opinion on a cleaner way to do it.

Please Log in or Create an account to join the conversation.

More
17 Nov 2012 17:04 - 17 Nov 2012 17:04 #2963 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
Well, I've setup the quick-pages now. In the progress I reworked the devo10 menu system so it isn't doing string-compares everywhere. I think it'll also be possible to merge some of the 'pages' code with the devo8 since much of it is now very similar, but I haven't gone that far yet.

Give it a try. If you still really see value in menu configuration, I'll back down on it. I don't think it is at all necessary, but the more I think about it, I agree that if some people think it really brings value, then I'll support it.

You'll need to rework it to work with the current menu changes though.
Last edit: 17 Nov 2012 17:04 by PhracturedBlue.

Please Log in or Create an account to join the conversation.

More
18 Nov 2012 11:17 #2975 by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back(2)

PhracturedBlue wrote: There are 3 options here:
1) Convince me I'm wrong. All it will take is a compelling argument for why this will significantly help end-users vs the alternatives. So far I haven't seen anything significant enough to sway me.
2) Live with my decision
3) Fork the project and go it your own way.

In our real life, other than wrong-or-right, black-or-white, there are many cases in which no one is wrong. Hence I don't think your above options are valid to this case.
In this case, firstly I do agree that you, as the major contributor to this great project, have the right to let all features meet your need. However, if we want to make the project have a better user-experience, we should provide features meet the majority of users needs as well. The more users test-use this project, the more stable it will be, eventually benefit you.
Providing a customizable menu doesn't turn down your preference about the TeleMonior, but gives other users better experience. That is why I choose to do it.
Of coz, you current solution is also good. Currently, I already had my repo synced with your latest changes, and fixed bugs related to the quick page, please check them out.
However, I still have 2 questions:
1) Should it be TX-specific instead? The main-page-config is obviously model-specific as DSM2 models need to monitor CH1 while Devo models need to monitor CH3. But the quickpage is most likely used for all models, putting it to modelxx.ini just means we have to setup the same quickpage many times for all model we used
2) DEVO8 is a touch-oriented device, so many keys are available. But in devo10, all keys are used in most pages, using the right/left keys, or any other keys, will interfere with normal key actions. I am thinking of using long-press for switching quickpages, but I still see issues about how to determine interval between 2 long-pressing.

Please Log in or Create an account to join the conversation.

More
18 Nov 2012 12:10 #2976 by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back(2)
Other codes need your review
1) I revert your change in the mixer-page. In devo10, using show/hide to dynamically draw a page is dangerous. With your change, the mixer-page will crash whenever a mixer is changed from none to simple/expo/complex. Using logical view can avoid the crash, however, it makes page-scrolling very hard to implement and lower down codes' readability.
2) In devo10's mixer-setup and mixer-curve, I add codes allow users to long-press ENT to save change in partially live without exiting the page. With current design, setting up a model for Expo&DR is very inconvenient: enter the mixer-setup page ->enter curve page-> save and exit to mixer-setup-> change other settings -> save and exit to mixer-page-> verify servos' response-> repeat above actions by entering/exiting pages back and forth. I understand that providing save-in-live might slow down event-handling performance and reduce flash's life, so this feature is a workaround to simplified mixer setup a lot. Please let me know if you see any issue about it or come up with a better solution

Please Log in or Create an account to join the conversation.

More
18 Nov 2012 13:24 - 18 Nov 2012 13:42 #2977 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
Note. you don't need to use left/right for the quickpages. you could use a set of trim buttons if you like. that is why it is configurable. adding support for a long-press would be cool too (turn the spinbox into a pressable spinbox to toggle from regular to long-press activation)

Other codes need your review
1) I revert your change in the mixer-page. In devo10, using show/hide to dynamically draw a page is dangerous. With your change, the mixer-page will crash whenever a mixer is changed from none to simple/expo/complex. Using logical view can avoid the crash, however, it makes page-scrolling very hard to implement and lower down codes' readability.

I see no crash. and I'm not show/hiding anything, just calculating whether or not to waste the extra line when it isn't needed.
Please describe exactly how to reproduce the crash.

2) In devo10's mixer-setup and mixer-curve, I add codes allow users to long-press ENT to save change in partially live without exiting the page. With current design, setting up a model for Expo&DR is very inconvenient: enter the mixer-setup page ->enter curve page-> save and exit to mixer-setup-> change other settings -> save and exit to mixer-page-> verify servos' response-> repeat above actions by entering/exiting pages back and forth. I understand that providing save-in-live might slow down event-handling performance and reduce flash's life, so this feature is a workaround to simplified mixer setup a lot. Please let me know if you see any issue about it or come up with a better solution

I don't think you should be saving anything (is there a specific need for this?)EDIT: I looked at the code. No saving to flash is being done, but having long-press copy-back to the main mixer is probably ok. The addition of the bar-graph was supposed to help. It should show the live channel behavior. Is it not sufficient, or you just want to see real servo response? the reason for this page not being live is only because you need to cylce through invalid options to get to legal ones. Things like setting the input and switch should not be live. It would probably be ok for everything else to be live though. On the complex page, changing the # of pages and the mux type also shouldn't happen live.
Last edit: 18 Nov 2012 13:42 by PhracturedBlue.

Please Log in or Create an account to join the conversation.

More
18 Nov 2012 13:35 #2978 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)

suvsuv wrote: However, I still have 2 questions:
1) Should it be TX-specific instead? The main-page-config is obviously model-specific as DSM2 models need to monitor CH1 while Devo models need to monitor CH3. But the quickpage is most likely used for all models, putting it to modelxx.ini just means we have to setup the same quickpage many times for all model we used

It is a tradeoff. For instance, a model with telemetry wants the telemetry page available, but others don't. If I have GPS, i may want it included too. I think it is exactly the same as the model-config page. you want to keep the same settings for most models, but some models need a different variant, and the cost is setting it up every time. I'd live to solve that, but it isn't easy.
The 3 leading contenders seem to be:
1) provide a mecahnism to copy the display settings from one model to another
2) use separate files for the display info, and allow multiple models to use the same file (such that a change made in one changes all)
3) allow setting a default for the Tx, and only save the differences to the model file.
Every one of these has downsides associated with it, though I'd probably lead towards the third option.

2) DEVO8 is a touch-oriented device, so many keys are available. But in devo10, all keys are used in most pages, using the right/left keys, or any other keys, will interfere with normal key actions.

FYI, the Devo8 is fully accessible by button only. It does not require the touch interface. But as you say, if you use left/right to go-to a page needing the left/right buttons, you can only get back with 'exit'. Which is why I support changing which buttons are used. But as I said, I like the idea of being able to use long-press too.

Please Log in or Create an account to join the conversation.

More
18 Nov 2012 13:38 #2979 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)

suvsuv wrote: Providing a customizable menu doesn't turn down your preference about the TeleMonior, but gives other users better experience. That is why I choose to do it.

And to lay this matter to rest, I already said that if you still think it is valuable, I'll take it. We don't need to argue about it anymore.

Please Log in or Create an account to join the conversation.

More
18 Nov 2012 14:39 #2980 by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back(2)

PhracturedBlue wrote: I see no crash. and I'm not show/hiding anything, just calculating whether or not to waste the extra line when it isn't needed.
Please describe exactly how to reproduce the crash.

Reproduce steps:
1) run the Emu_devo10, enter the mixer setup page for THR(CH1), change template from simple to Cyclic1, hit save button
the change is saved and back to mixer page
2) prese up or down,the emu_devo10 crashes.
3) If you repeat above steps in real devo10, the mixer page won't crash directly, but you will see the mixer page won't be refreshed after pressing the down key for several times. At this moment, memory violation actually happens, you will see the GUI crashes eventually and then reboot if you keep pressing the down key.

Please Log in or Create an account to join the conversation.

More
18 Nov 2012 14:48 #2981 by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back(2)

PhracturedBlue wrote: I don't think you should be saving anything (is there a specific need for this?)EDIT: I looked at the code. No saving to flash is being done, but having long-press copy-back to the main mixer is probably ok. The addition of the bar-graph was supposed to help. It should show the live channel behavior. Is it not sufficient, or you just want to see real servo response? the reason for this page not being live is only because you need to cylce through invalid options to get to legal ones. Things like setting the input and switch should not be live. It would probably be ok for everything else to be live though. On the complex page, changing the # of pages and the mux type also shouldn't happen live.

Yes, I want to see servos' response. The bar-graph is helpful but can't reflect servos' actual behavior all the time, e.g. when mixer settings exceeds actual endpoint's limit of a servo.
My current solution is not going to make the change be in live, but just provide a way to save changes on demand and without exiting from current page and

Please Log in or Create an account to join the conversation.

More
18 Nov 2012 15:28 #2982 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)

suvsuv wrote:

PhracturedBlue wrote: I see no crash. and I'm not show/hiding anything, just calculating whether or not to waste the extra line when it isn't needed.
Please describe exactly how to reproduce the crash.

Reproduce steps:
1) run the Emu_devo10, enter the mixer setup page for THR(CH1), change template from simple to Cyclic1, hit save button
the change is saved and back to mixer page
2) prese up or down,the emu_devo10 crashes.
3) If you repeat above steps in real devo10, the mixer page won't crash directly, but you will see the mixer page won't be refreshed after pressing the down key for several times. At this moment, memory violation actually happens, you will see the GUI crashes eventually and then reboot if you keep pressing the down key.

Thanks, I've checked in a fix keeping my layout but fixing this issue.

Please Log in or Create an account to join the conversation.

More
18 Nov 2012 15:28 #2983 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
I think I've got all of your changes except the language updates.
Let me know if you think I missed anything.

Please Log in or Create an account to join the conversation.

More
19 Nov 2012 14:45 - 19 Nov 2012 14:47 #3002 by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back(2)
PB, I had my repo synced with yours, and found 2 bugs
1) The new timer you introduced in the fltk.c won't work for all Windows OSes, at least doesn't work in my Win7 and Win8 machines. In the ftlk.c, you assume that the OS can generate time tick as short as 1milliseconds, but in my test, the shortest tick is 5ms so it ruin many actions in emu_devo10, e.g., long-pressing doesn't work, binding time elapses every 5 seconds instead.
I fixed the issue and had it verified in my Windows machine. I don't know how it work in Linux, please verify the fix if you have Linux environment.

2) Your fix for the mixer page still doesn't work, I can still make the page crash in both emulator and real devo10， below is the repo steps:
click the last item of the mixer page(illustrated in the following snapshot) and change its template from none to simple, or Expo, after saving the change, the mixer page crashes



Also ,even though you can fix above issue, I don't think it actually meets your original expectation. You were trying to show as more items as possible in a page by hiding some of them based on their different template setting, but if you look at the following senario, I don't really think your current solution has advantage over fixed-row. As what I explained to you, it is not a good idea to design variable-rows page in devo10(though it works well in devo with sufficient LCD height)

Attachments:
Last edit: 19 Nov 2012 14:47 by suvsuv.

Please Log in or Create an account to join the conversation.

  1. Forum
  3. News, Announcements and Feedback
  5. Feedback & Questions
  7. PB, request for code review and merge back(2)
Time to create page: 0.105 seconds
Powered by Kunena Forum
Force desktop display