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
13 Nov 2012 12:59 - 13 Nov 2012 13:01 #2905 by FDR
Replied by FDR on topic PB, request for code review and merge back(2)

suvsuv wrote: 3) Have lang strings coincided as more as possible for devo8 and devo10


I see you've shortened the telemetry temperature unit from "Fahrenheit" to "Fahren" to fit in the box.
Why do not we simply use "°F" and "°C" instead? It would fit into an even smaller box too...


EDIT: ...and btw (I know it wasn't you) it is "Celsius", not "Celcius". ;)
Last edit: 13 Nov 2012 13:01 by FDR.

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

More
13 Nov 2012 13:02 #2906 by xCometz
Replied by xCometz on topic PB, request for code review and merge back(2)

suvsuv wrote: The current is measured on battery supply.


That makes sense, LCD logic only need 2.5mA on 3.3V and backlight 15mA on 3.3V

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

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

suvsuv wrote: PB, please remove generating lang strings automatically from the makefile, it is annoying and might mask compile warnings. I prefer to type "make language" manually.

Can you explain the issue?
How will it mask compile warnings?
What trouble is it causing you besides a little extra runtime?
It should not output anything to the screen. If it does than something is wrong.

'make language' does something completely different than what a normal 'make' does.
'make language' rebuilds the 'master' language files with updated strings.
'make' splits the master language files into per-model language files.

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

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

sbstnp wrote: IMO telemetry should stay where it is currently, not every model we fly has telemetry, and if we want quick acces, we could implement a shortcut system and bind the page to the R or L keys.

Well, I decided differently for now. Once we have a configurable hot-key system in place, We can move it back.

But in the end I since nobody commented on the issue I raised on bitbucket I guess nobody cares enough, so whatever flies your model :). I'll probably just patch the code the way I like it.

You need to be patient :) Just because I don't respond quickly doesn't mean I don't care. It usually just means I'm focusing on something else at the moment.

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

More
13 Nov 2012 14:16 - 13 Nov 2012 14:20 #2910 by vlad_vy
Replied by vlad_vy on topic PB, request for code review and merge back(2)

FDR wrote: Sorry, I had no time at all for it, not even for updating the language file.
Besides, you have discouraged me about the placements...


Sorry, but with russian it's impossible to do short translation for 'Prealert time:', Prealert interval:' and 'Timeup interval'. 'Telemetry temp:' and 'Telemetry unit:' also are quite long. If I restate and shorten strings, that havn't sense without descriptive headers. For example:

Telemetry Units
Temperature: []
Length: []

Timer settings
Prealert time: []
Prealert interval: []
Timeup interval: []

I like previous Tx 'Configure' placement, all Tx related setting is placed at one screen. Telemetry and timer settings can be placed at second part of page. Also, many free space at first part of Devo8 Tx 'Configure' page rather mislead, I don't wait any continuation and always press 'Next' page button.
Last edit: 13 Nov 2012 14:20 by vlad_vy.

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

More
13 Nov 2012 14:37 - 13 Nov 2012 14:37 #2911 by RandMental
Replied by RandMental on topic PB, request for code review and merge back(2)
I like the example, may I further suggest for English:

Telemetry Models Units
Temperature: []
Length: []

Flight Timer Settings
Prealert time: []
Prealert interval: []
Timeup interval: []
Last edit: 13 Nov 2012 14:37 by RandMental.

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

More
13 Nov 2012 15:20 #2912 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
I have reorganized the page to look like the Devo10 (it is now on 3 scrolled pages)
I really don't want more pages on the Tx tab. All of the other 'Tx' pages are for monitoring. There is only one used for configuration, and the amount of time you spend there will be very small.
So I am convinced the scrollbar is the right answer for transmitter configuration.
Hopefully this provides sufficient space for translation. If not, I can space out some lines further to allow 2-lines of description for some items.

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

More
13 Nov 2012 15:47 #2913 by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back(2)
PB, just fix 1 critical bug and 1 minor display issue in devo10, please check them out

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

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

PhracturedBlue wrote: You need to be patient :) Just because I don't respond quickly doesn't mean I don't care. It usually just means I'm focusing on something else at the moment.


I try :)

TBH because my life is moving so fast and hectic sometimes I expect everyone to be the same. Apologies to everyone I offended, didn't mean to.
Devo 10 + 4in1
Spektrum Dx9
FrSky Taranis + TBS Crossfire

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

More
16 Nov 2012 14:46 #2934 by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back(2)
1) Fix 1 memory violation bug in model name editing
2）Reduce unnecessary view refreshing in Expo&DR template editing
3) Refactor the mixer_page.c(PB, your tweak in this page is not able to resolve the issue)



4) Refactor the main menu and submenu, allow users to customize their own main menu items(putting Telem monitor into the main menu is a subjective design since many users might not RXes supporting Telemetry).
5) Upate Lang.cn and Lang.tw

PB, please take time to review them
Attachments:

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

More
16 Nov 2012 14:57 - 16 Nov 2012 14:58 #2937 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
I don't really like being able to configure the main menu. I really don't see it buying any benefit, and it makes documentation difficult/confusing. If you don't like the telemetry being there, then what I'd really want is the ability to configure buttons on the main menu (so you can have left/right cycle through whatever pages you want, say telemetry monitor and output monitor)
that would be helpful on both the devo8 and devo10
Last edit: 16 Nov 2012 14:58 by PhracturedBlue.

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

More
16 Nov 2012 15:08 #2938 by sbstnp
Replied by sbstnp on topic PB, request for code review and merge back(2)

PhracturedBlue wrote: I don't really like being able to configure the main menu. I really don't see it buying any benefit, and it makes documentation difficult/confusing.


I don't think it will make documentation anymore difficult. There are some items which are unmovable, like model setup and tx configuration, so you can only add new items, shortcuts if you like. The main layout is still fixed and documentation should not be affected.
Devo 10 + 4in1
Spektrum Dx9
FrSky Taranis + TBS Crossfire

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

More
16 Nov 2012 15:14 #2940 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
none the less, I don't expect to take suvsuv's current changes as is. it will add a change to the model.ini which I'll just need to remove later, menaning less compatibility going forward. being able to switch between pages from the main-page is much more convenient, and will be usable by both devo8 and devo10, meaning the model configuration is portable. Then we can additionally be more intelligent (like not having access to telemetry pages when telemetry isn't supported by the protocol). At that time, I'll remove telemetry from the main menu.

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

More
16 Nov 2012 15:18 #2941 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
FYI, since suvsuv already created the page config, reworking it to control the main-page instead of the main menu should not be too hard. I'll probably work on it today.

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

More
16 Nov 2012 15:21 - 16 Nov 2012 15:21 #2942 by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back(2)

PhracturedBlue wrote: none the less, I don't expect to take suvsuv's current changes as is. it will add a change to the model.ini which I'll just need to remove later, menaning less compatibility going forward. being able to switch between pages from the main-page is much more convenient, and will be usable by both devo8 and devo10, meaning the model configuration is portable. Then we can additionally be more intelligent (like not having access to telemetry pages when telemetry isn't supported by the protocol). At that time, I'll remove telemetry from the main menu.

If you don't want to see it, just comment out 2 lines in the menu.c. The change is not in model.ini but in tx.ini.
But I do see the benefit of the feature I got good feedback from devo10 users, the menu it devo10-specific and I don't agree that devo10 must be exactly the same as devo 8.

Attachments:
Last edit: 16 Nov 2012 15:21 by suvsuv.

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

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

suvsuv wrote: and I don't agree that devo10 must be exactly the same as devo 8.


I agree, with Devo 10 being more limited in accessibility than Devo 8 any extra customization should be welcome :)
Devo 10 + 4in1
Spektrum Dx9
FrSky Taranis + TBS Crossfire
Last edit: 16 Nov 2012 15:40 by sbstnp.

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

More
16 Nov 2012 15:38 - 16 Nov 2012 15:40 #2944 by FDR
Replied by FDR on topic PB, request for code review and merge back(2)
Off topic

@PB there is an error compiling the emu_devo8:
+ Compiling 'target/common_emu/fltk.cpp'
target/common_emu/fltk.cpp:26:19: fatal error: FL/Fl.H: No such file or director
y
compilation terminated.
make: *** [objs/emu_devo8/fltk.o] Error 1


EDIT: ...and in the emu_devo10 too:
+ Compiling 'target/common_emu/fltk.cpp'
target/common_emu/fltk.cpp:26:19: fatal error: FL/Fl.H: No such file or director
y
compilation terminated.
make: *** [objs/emu_devo10/fltk.o] Error 1
Last edit: 16 Nov 2012 15:40 by FDR.

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

More
16 Nov 2012 15:43 #2945 by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back(2)

FDR wrote: Off topic

@PB there is an error compiling the emu_devo8:

+ Compiling 'target/common_emu/fltk.cpp'
target/common_emu/fltk.cpp:26:19: fatal error: FL/Fl.H: No such file or director
y
compilation terminated.
make: *** [objs/emu_devo8/fltk.o] Error 1


EDIT: ...and in the emu_devo10 too:
+ Compiling 'target/common_emu/fltk.cpp'
target/common_emu/fltk.cpp:26:19: fatal error: FL/Fl.H: No such file or director
y
compilation terminated.
make: *** [objs/emu_devo10/fltk.o] Error 1

You should type "make TARGET=emu_devo8 WINDOWS=1"
When you miss the "WINDOW=1", you get the above errors

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

More
16 Nov 2012 15:49 #2946 by FDR
Replied by FDR on topic PB, request for code review and merge back(2)
Jeez! I mistyped, thanks!

WINDEOWS :lol:

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

More
16 Nov 2012 17:09 - 16 Nov 2012 17:10 #2947 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
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.
Last edit: 16 Nov 2012 17:10 by PhracturedBlue.

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.115 seconds
Powered by Kunena Forum
Re-enable mobile display