- Posts: 268
- Forum
- News, Announcements and Feedback
- Feedback & Questions
- PB, request for code review and merge back
PB, request for code review and merge back
- suvsuv
-
Topic Author
- Offline
Less
More
17 Oct 2012 10:31 #2301
by suvsuv
PB, request for code review and merge back was created by suvsuv
PB, I already had my repo re-orged so that the folder structure now align with yours. Could you please spend some time to review codes in my repo and let me know whether they can be merged back to your repo? I will still work on my own repo in the future but the merge will save me some efforts when syncing with your updates.
Here is more detail info to help you review my changes
1) Devo6/8 use the pages folder , and Devo10 use the pages-devo10 folder during compiling. This is decided in the Makefile, hence other people could plug any pages-xxx for their preferred GUI pages
2) Other codes have very minimal modification to support devo8/6 and devo10 by using preprocessor directives, which makes the deviation project be compatible with any types of Devo TX
3) I don't change any functional methods for devo8 and devo6, dfus for devo8/6 should work exactly the same as yours. I verify most features in the emu_devo8, however, I don't have a real devo8 to test.
Here is more detail info to help you review my changes
1) Devo6/8 use the pages folder , and Devo10 use the pages-devo10 folder during compiling. This is decided in the Makefile, hence other people could plug any pages-xxx for their preferred GUI pages
2) Other codes have very minimal modification to support devo8/6 and devo10 by using preprocessor directives, which makes the deviation project be compatible with any types of Devo TX
3) I don't change any functional methods for devo8 and devo6, dfus for devo8/6 should work exactly the same as yours. I verify most features in the emu_devo8, however, I don't have a real devo8 to test.
Please Log in or Create an account to join the conversation.
- PhracturedBlue
-
- Offline
Less
More
- Posts: 4402
17 Oct 2012 13:55 #2302
by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back
I am planning to take a look as soon as I am able to.
Thanks again for working on this.
Thanks again for working on this.
Please Log in or Create an account to join the conversation.
- PhracturedBlue
-
- Offline
Less
More
- Posts: 4402
17 Oct 2012 14:17 #2303
by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back
Ok, I spent a little while looking at it. The concepts and code itself basically looks fine but...
Because you manually merged my code rather than doing it through mercurial, it will be impossible for me to use a pull to import your changes. But in this case I probably wouldn't anyway so it doesn't matter much.
I want to remove all of the #ifdefs in the code, and will need to rework things so that they are unnecessary, or at least are based on capabilities rather than model number.
The filesystem layout needs to be redone so that the filesystem is built for each Tx, rather than having both devo8 and devo10 files in the same directory.
I'm a bit concerned with fully separating the 'pages' as you've done. I understand the need, but am hopeful that at least some of the core functions can be shared. There is a lot of complexity in those files, and needing to maintain multiple copies will be a heavy burden.
Because you manually merged my code rather than doing it through mercurial, it will be impossible for me to use a pull to import your changes. But in this case I probably wouldn't anyway so it doesn't matter much.
I want to remove all of the #ifdefs in the code, and will need to rework things so that they are unnecessary, or at least are based on capabilities rather than model number.
The filesystem layout needs to be redone so that the filesystem is built for each Tx, rather than having both devo8 and devo10 files in the same directory.
I'm a bit concerned with fully separating the 'pages' as you've done. I understand the need, but am hopeful that at least some of the core functions can be shared. There is a lot of complexity in those files, and needing to maintain multiple copies will be a heavy burden.
Please Log in or Create an account to join the conversation.
- suvsuv
-
- Offline
Less
More
- Posts: 268
17 Oct 2012 14:56 #2307
by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back
No problem, if you come up a way to let devo10 and devo8 share some common UI pages, that couldn't be better. I will continue on writing codes for devo10 and looking forward to your refactored result
Please Log in or Create an account to join the conversation.
- suvsuv
-
- Offline
Less
More
- Posts: 268
17 Oct 2012 15:04 - 17 Oct 2012 15:08 #2309
by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back
One more question: How come you couldn't pull codes from my repo?
I submit my codes using the SourceTrees, that is a GUI mercurial client, and I can pull codes from my repo in my another virtual machine
I submit my codes using the SourceTrees, that is a GUI mercurial client, and I can pull codes from my repo in my another virtual machine
Last edit: 17 Oct 2012 15:08 by suvsuv.
Please Log in or Create an account to join the conversation.
- PhracturedBlue
-
- Offline
Less
More
- Posts: 4402
17 Oct 2012 15:40 #2312
by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back
I mean that the merge capabilities of mercurial are not useful with your workmodel, not that I can't checkout your code.
Please Log in or Create an account to join the conversation.
- PhracturedBlue
-
- Offline
Less
More
- Posts: 4402
18 Oct 2012 05:06 #2324
by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back
Here is my plan for how to make the merge easier. I've cloned your repository, and will start modifying it to met the structure I think will be most maintainable, while keeping the functionality you already have.
I plan to work on it piece-by-piece, so I'll start with removing all the ifdefs from the gui code, for instance.
Once I have a section working, I'll merge it back into the deviation trunk, and go to the next piece. I expect to deal with the pages code last, because I don't really have any idea how to do it yet.
I plan to work on it piece-by-piece, so I'll start with removing all the ifdefs from the gui code, for instance.
Once I have a section working, I'll merge it back into the deviation trunk, and go to the next piece. I expect to deal with the pages code last, because I don't really have any idea how to do it yet.
Please Log in or Create an account to join the conversation.
- PhracturedBlue
-
- Offline
Less
More
- Posts: 4402
18 Oct 2012 06:13 #2325
by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back
I just checked in a proof of concept for the bargraph.
I see you've taken some shortcuts in the bargraph code.
specifically:
1) you draw outside the object's bounding box
2) you use a heuristic to draw 100% lines
I haven't fixed these yet, but they certainly both must go.
I see you've taken some shortcuts in the bargraph code.
specifically:
1) you draw outside the object's bounding box
2) you use a heuristic to draw 100% lines
I haven't fixed these yet, but they certainly both must go.
Please Log in or Create an account to join the conversation.
- PhracturedBlue
-
- Offline
Less
More
- Posts: 4402
18 Oct 2012 06:17 #2326
by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back
I just realized. Maybe it'd be better if I do the cleanup directly in your repository, rather than having yet another for this task. That is up to you if you want me committing to your repo or not. Just let me know.
Please Log in or Create an account to join the conversation.
- PhracturedBlue
-
- Offline
Less
More
- Posts: 4402
18 Oct 2012 21:51 #2327
by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back
I've pushed my latest changes into the cleanup branch.
I have finished refactoring the gui code. I will likely merge it into the deviation trunk next, but I'll be traveling for a couple days, so that may not happen right away.
I have finished refactoring the gui code. I will likely merge it into the deviation trunk next, but I'll be traveling for a couple days, so that may not happen right away.
Please Log in or Create an account to join the conversation.
- PhracturedBlue
-
- Offline
Less
More
- Posts: 4402
19 Oct 2012 16:17 #2332
by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back
I've merged the Devo10 work done by suvsuv into the main branch now.
I didn't yet try to consolidate the devo8 and devo10 pages, and there is still a little cleanup to do here and there, but mostly it seems to work well.
I didn't yet try to consolidate the devo8 and devo10 pages, and there is still a little cleanup to do here and there, but mostly it seems to work well.
Please Log in or Create an account to join the conversation.
- FDR
-
- Offline
19 Oct 2012 16:30 #2333
by FDR
Replied by FDR on topic PB, request for code review and merge back
Compiling the devo8 emulator throws an error:
+ Compiling 'target/common_emu/fltk.cpp'
In file included from target/common_emu/fltk.h:4:0,
from target/common_emu/fltk.cpp:37:
./gui/gui.h:42:5: error: expected identifier before numeric constant
./gui/gui.h:42:5: error: expected '}' before numeric constant
./gui/gui.h:42:5: error: expected unqualified-id before numeric constant
target/common_emu/fltk.cpp:40:1: error: expected declaration before '}' token
make: *** [objs/emu_devo8-w32/fltk.o] Error 1Please Log in or Create an account to join the conversation.
- PhracturedBlue
-
- Offline
Less
More
- Posts: 4402
19 Oct 2012 17:33 #2339
by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back
try removing the 'objs' dir. It compiles fine for me even on a clean pull. Haven't checked on Windows. Maybe something different there. I'll take a look
Please Log in or Create an account to join the conversation.
- FDR
-
- Offline
19 Oct 2012 17:39 #2340
by FDR
Replied by FDR on topic PB, request for code review and merge back
No, deleting the objs dir didn't help...
Please Log in or Create an account to join the conversation.
- PhracturedBlue
-
- Offline
Less
More
- Posts: 4402
20 Oct 2012 18:31 #2353
by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back
It is resolved now.
Please Log in or Create an account to join the conversation.
- FDR
-
- Offline
20 Oct 2012 19:21 #2355
by FDR
Replied by FDR on topic PB, request for code review and merge back
Yep, it is...
Please Log in or Create an account to join the conversation.
- suvsuv
-
- Offline
Less
More
- Posts: 268
21 Oct 2012 11:10 #2359
by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back
Thans PB. The merged codes look much cleaner 
I now had my repo in synced with your latest update and start working on mixer pages. Currently, I add 1 flag(traditional_mixer=1) in all devo10's model ini files , by this way, each model will be able to use either advanced mixers(aka deviation mixers) or traditional mixers(subtrim, travel adj, swash mix, thro curves and pit curves).
I now had my repo in synced with your latest update and start working on mixer pages. Currently, I add 1 flag(traditional_mixer=1) in all devo10's model ini files , by this way, each model will be able to use either advanced mixers(aka deviation mixers) or traditional mixers(subtrim, travel adj, swash mix, thro curves and pit curves).
Please Log in or Create an account to join the conversation.
- PhracturedBlue
-
- Offline
Less
More
- Posts: 4402
21 Oct 2012 12:08 #2361
by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back
I probably won't pick that stuff up unless you want to write an interface for the devo8 too.
One of the tenets of Deviation is that you can share models between any Tx running deviation. a) I'm not keen on having 2 completely different mixers, and b) not being able to configure the same parameters on all Tx is certainly a showstopper.
If the mixers aren't compatible, then users who pick up a model will not be able to switch between them which I don't like either.
So if this is the direction you want to go, it is likely where you maintain your own fork. I am fine with that, it is why I chose the license I did. I'll start working on an interface to the existing mixers for the Devo10.
One of the tenets of Deviation is that you can share models between any Tx running deviation. a) I'm not keen on having 2 completely different mixers, and b) not being able to configure the same parameters on all Tx is certainly a showstopper.
If the mixers aren't compatible, then users who pick up a model will not be able to switch between them which I don't like either.
So if this is the direction you want to go, it is likely where you maintain your own fork. I am fine with that, it is why I chose the license I did. I'll start working on an interface to the existing mixers for the Devo10.
Please Log in or Create an account to join the conversation.
- suvsuv
-
- Offline
Less
More
- Posts: 268
21 Oct 2012 15:27 #2366
by suvsuv
Replied by suvsuv on topic PB, request for code review and merge back
Don't be too serious, PB 
, I will firstly port your deviation mixer to devo10. It should be the fastest and most easy way to let your great framework shared with Devo10 user, since all gui widgets are working in Devo10 now. Hopefully ,the 1st beta version will be done in 2 weeks
, I will firstly port your deviation mixer to devo10. It should be the fastest and most easy way to let your great framework shared with Devo10 user, since all gui widgets are working in Devo10 now. Hopefully ,the 1st beta version will be done in 2 weeks Please Log in or Create an account to join the conversation.
- PhracturedBlue
-
- Offline
Less
More
- Posts: 4402
22 Oct 2012 00:54 #2369
by PhracturedBlue
Replied by PhracturedBlue on topic PB, request for code review and merge back
Sorry, I am severely jet-lagged at the moment and my communication skills are not at their finest. First, I absolutely appreciate the work you've done. Second, I am also fully in support of an easier interface for the setup, but what I'd like to see would be to map that interface onto the existing mixer infrastructure (I'm thinking that you have a fixed number of mixers, each defined with a specific function that allows you to map classic functionality to each channel). I'm not sure it is possible to get the functionality you are after, but if it is, then for the same model.ini file, you could switch from the basic to advanced interface and still have all your settings. It would also mean only one mixer file with only one set of bugs to work out.
Please Log in or Create an account to join the conversation.
- Forum
- News, Announcements and Feedback
- Feedback & Questions
- PB, request for code review and merge back
Time to create page: 0.060 seconds
-
Home
-
Forum
-
News, Announcements and Feedback
-
Feedback & Questions
- PB, request for code review and merge back