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

More
04 Nov 2012 07:21 #2666 by sbstnp
Suvsuv, PB, do you guys mind if I start creating issues for devo 10 branch? I'll try to help with the code too where I can/have time and send pull requests.

Devo 10 + 4in1
Spektrum Dx9
FrSky Taranis + TBS Crossfire

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

More
04 Nov 2012 13:26 - 04 Nov 2012 13:29 #2670 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
I'd appreciate it if you file issues on the deviation project (not suvsuv's branch). I have incorporated virtually all of his work into the trunk, and the trunk is what we'll end up releasing.
It would be better if you could figure out why your build system doesn't work.
Releasing binaries of every change is quite time consuming.
If you can post details of what you have, we can start trying to figure out what is wrong.
Please start a new thread for that.

For instance, at least some of the bugs you've filed have already been addressed. Being able to run the latest code, really is critical.
Last edit: 04 Nov 2012 13:29 by PhracturedBlue.

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

More
04 Nov 2012 13:45 - 04 Nov 2012 13:50 #2671 by sbstnp

PhracturedBlue wrote: I'd appreciate it if you file issues on the deviation project (not suvsuv's branch). I have incorporated virtually all of his work into the trunk, and the trunk is what we'll end up releasing.
It would be better if you could figure out why your build system doesn't work.
Releasing binaries of every change is quite time consuming.
If you can post details of what you have, we can start trying to figure out what is wrong.
Please start a new thread for that.

For instance, at least some of the bugs you've filed have already been addressed. Being able to run the latest code, really is critical.


My build system is fine, I couldn't get it running using my distro's repository, so I resorted to summon-arm-toolchain and everything is fine. I run Archlinux and there are a number of ARM toolchains in the Archlinux User Repository. Unfortunately none worked ok, so I had to dump them.

Will move issues from suvsuv's repo to main, should I mention which commit is my build based on, as a reference and for history's sake? Should not matter, I always update before building a dfu, but if it makes it easier for you guys, it's not an effort on my part.

Devo 10 + 4in1
Spektrum Dx9
FrSky Taranis + TBS Crossfire
Last edit: 04 Nov 2012 13:50 by sbstnp. Reason: Typos.

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

More
04 Nov 2012 14:04 #2672 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
Good to hear. Yes, it is quite helpful to know what version you are running in a ticket. I have just synced all of suvsuv's code (except for the main loop and the channel ordering). I should get to the channel-ordering today. The main loop stuff I need to think about some more.

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

More
04 Nov 2012 14:57 #2674 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
I just applied the latest Devo10 code to my Tx. Committed the channel reordering stuff.
I have seen the Tx reboot a couple times on the main screen which means we're hitting the watchdog. The screen is definitely a lot slower on the Devo10 than on the 8. I wonder if this is related.

Besides some updates I did, the only difference between my code and suvsuv's is the main page timing.

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

More
04 Nov 2012 15:30 #2676 by suvsuv
it is weird you saw rebooting several time , Please make a build under my repo(update again as your current codes is not in synced with my repo) and have it downloaded to your TX, see if the TX still reboots from time to time.
In terms of slow refreshing compared to devo8, I am sure it not related to the main loop as the GUI_RefreshScreen frequency is the same as devo8's , I believe it should related to the following codes in the lcd.c, your previous clear logic is buggy and I don't have time to figure it out, just using a work-around now

void LCD_Clear(unsigned int val)
{
/* int i,j;
val = (val & 0xFF);
for (i=0; i<LCD_PAGES; i++) {
lcd_set_page_address(i);
lcd_set_column_address(0);
for (j=0;j<PHY_LCD_WIDTH;j++)
LCD_DATA = val;
} */
// Bug fix: above method doesn't work properly, especially during keyboard type changing.
LCD_FillRect(0, 0, LCD_WIDTH, LCD_HEIGHT, val);
}

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

More
04 Nov 2012 15:44 #2677 by sbstnp
Haven't seen any reboots in my ~3 hours total play time.

Devo 10 + 4in1
Spektrum Dx9
FrSky Taranis + TBS Crossfire

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

More
04 Nov 2012 15:47 #2678 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
I think Devo10 display is just slower to update than the devo8's. nothing to do with the code. Fixing the optimized clear may help, but that is only useful for changing pages, not the lag when updating values on a given page.

I've looked at your latest code (will merge shortly) but I don't think any of it will affect the watchdog timeout (except maybe the stuff in main, which I'm not ready to take).

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

More
04 Nov 2012 16:02 #2679 by suvsuv
Rebooting might not only caused by watchdog, but also using NULL pointers. I believe it is most likely the latter

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

More
04 Nov 2012 17:04 #2680 by rangebound
Replied by rangebound on topic PB, request for code review and merge back(2)
Hi guys,

Many thanks for all your efforts - just too cool and generous even for an emoticon.

I've opened a few issues with along with the build ID. Let me know (here) if I did wrong and I'll stop.

BTW no reboots for me as yet.

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

More
05 Nov 2012 14:53 - 05 Nov 2012 14:53 #2698 by suvsuv
PB, please do a code review and merge back again. You should have a look at main.c and take bug fix on backlight and contrast setting from tx.ini. Other changes include:
1) Fix all devo10 bugs raised in the BitBucket
2)Refactor listbox widget.
3) Don't allow user to interrupt binding until we figure out why the current goes up when interrupting binding
4) Refactor the Tx-configue page
5) Refactor the calibration procedure and let devo10 store calibration data for Aux4 and Aux5 knobs
6) Move protocol selection and binding into model_page, and hence remove all compile warnings
Last edit: 05 Nov 2012 14:53 by suvsuv.

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

More
05 Nov 2012 14:59 #2699 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
I'll work on it today. thanks.

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

More
05 Nov 2012 16:37 #2700 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
I'll work on it today. thanks.

You should not need to use different fonts for the devo8 and devo10. For instenace this is completely unnecessary:
+void _set_font()
+{
+    LCD_SetFont(DIALOGBODY_FONT.font);
+    LCD_SetFontColor(DIALOGBODY_FONT.font_color);
+
+}
Just change the font definition in the config.ini to meet your needs.
Also, I'm not planning to take the swapping the right and left keys. They are labeled as 'R' and 'L' and need to behave that way. Walkera has a weird definition of direction, for sure.
If you really don't like it, then swap the definition of R and L in channels.c for the Devo10

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

More
05 Nov 2012 17:34 - 05 Nov 2012 17:35 #2702 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
I've merged everything I'm going to for the moment.
Things I didn't take:
1) power on backlight in init code. this is necessary in the case that the filesystem doesn't exist or doesn't load properly. it will immediately be reset once the FS is loaded (on a color LCD, the screen is unreadable without backlight, and I think it should be on for displayingthe USB logo on the Devo10 too)
2) power savings in main.c. Still not convinced
3) dialog font stuff (see above post)
4) calibration (I don't understand why your changes are needed. Explain further, or wait until I can try it on my Tx)
5) use buttons to change brightness (I want the buttons reserved for now. I'll think on this)
6) keyboard (see above post)
7) disable binding kill. this makes the j6pro unusable. We need to fix the problem, not mask it with a band-aid
Last edit: 05 Nov 2012 17:35 by PhracturedBlue.

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

More
06 Nov 2012 00:52 #2708 by suvsuv

PhracturedBlue wrote: I've merged everything I'm going to for the moment.
Things I didn't take:

4) calibration (I don't understand why your changes are needed. Explain further, or wait until I can try it on my Tx)
5) use buttons to change brightness (I want the buttons reserved for now. I'll think on this)
6) keyboard (see above post)


4) In the calibration, I remove the 1st step which is meaningless.
5)in the current main page, the R/L buttons are not used, so they could be used for short-cut keys. Please come up with more options , I could create a shortcut key config page to let users decide.
6) I would insist on swapping the R/L behavior in the real devo10, please don't make your judgment by playing with devo8 or emu_devo10. In the real devo10 device, the R key points to the left while the L key points to the right, please do test the keyboards in a real devo10

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

More
06 Nov 2012 00:57 #2709 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
for (5) Deviation supports virtual channels which can be mapped to the up/down left/right buttons. This is why those buttons are not currently mapped in Deviation (until you press Ent to enter the menu).
It is likely virtually noone uses that capability, so maybe we should allow configuration for other 'hotkey' like functions, but atthe moment thatis why they are reserved, and why I'm hesitant to dedicate them to the backlight.

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

More
06 Nov 2012 00:58 #2710 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
for (6) if you really want the right and left buttons to behave in reverse, change them in channels.c so they are consistent everywhere.

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

More
06 Nov 2012 01:52 #2711 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
Ok. I've used the keyboard on the Devo10 now. Understandably you don't want to swap the R/L buttons everywhere since in most cases 'R' means 'up'. It is a little counter-intuitive initially to have the R button move Right on the keyboard, but after hitting it twice, you get over that and it feels perfectly natural to me. So I'm not going to take your changes on this one at the moment. If there is violent uproar, I may change my mind, but for now I'm leaving my FW as in.

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

More
06 Nov 2012 02:05 #2712 by suvsuv

PhracturedBlue wrote: for (6) if you really want the right and left buttons to behave in reverse, change them in channels.c so they are consistent everywhere.

Make sense to me, will do it this way。
If you compared the orientation of the L/R keys in both devo8 and devo10, you will understand why I insist on having the R/L swapped. Walkera guys are just lazy and stupid



Attachments:

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

More
06 Nov 2012 02:15 #2714 by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back(2)
As I said. I've used it. It really doesn't feel that weird to me.

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

Time to create page: 0.122 seconds
Powered by Kunena Forum