add ComboBlade and associated wrappers #668
Conversation
KeeganMorrow
force-pushed
the
combo_blade_pr
branch
2 times, most recently
from
8f2a33a to
200a0b4
Compare
Thanks for the feedback, I made some updates based on your comments. |
blades/combo_blade.h
Outdated
| return real_style_->NoOnOff(); | ||
| } | ||
|
|
||
| virtual bool Charging() { return real_style_->Charging();} |
blades/combo_blade.h
Outdated
| } | ||
|
|
||
| int num_leds() const override { | ||
| int result = blade1_num_leds_; |
There was a problem hiding this comment.
this would be shorter and easier to read:
return blade1_num_leds + blade2->num_leds();
blades/combo_blade.h
Outdated
| } | ||
|
|
||
| BladeStyle* UnSetStyle() override { | ||
| if (dummy_style_) |
There was a problem hiding this comment.
If you call blade_ / blade2_->UnsetStyle() here then dummy_style will never have a null style (when used), and we can drop the if() statements in that class.
There was a problem hiding this comment.
Moving these into a single object resolves this, thanks!
blades/combo_blade.h
Outdated
| blade2_(blade2) | ||
| { | ||
| blade_ = blade1; | ||
| dummy_style_ = new ComboBladeStyleWrapper(this, blade1); |
There was a problem hiding this comment.
ProffieOS code avoids using 'new' as much as possible to avoid memory fragmentation.
Just make "dummy_style" a ComboBladeStyleWrapper instead of a ComboBladeStyleWrapper*.
blades/combo_blade.h
Outdated
|
|
||
| void setRealStyle(BladeStyle* style){real_style_ = style;} | ||
|
|
||
| BladeStyle* GetRealStyle(){return real_style_;} |
There was a problem hiding this comment.
needs 3 more more spaces, should be:
BladeStyle* GetRealStyle() { return real_style_; }
(same comment applies in other places as well.)
KeeganMorrow
force-pushed
the
combo_blade_pr
branch
from
d1638f4 to
c30ae69
Compare
|
|
||
| BladeStyle* UnSetStyle() override { | ||
| BladeStyle* result = real_style_; | ||
| real_style_ = nullptr; |
There was a problem hiding this comment.
Should also call blade_ / blade2_ -> UnSetStyle() here.
There was a problem hiding this comment.
I'm not sure why, but it actually seems like I get a crash due to a null pointer somewhere when I tried this, willing to explore more if you think this idea is still worth pursuing
There was a problem hiding this comment.
If you have a config file that crashes (and instructions for how to make it crash if needed) I can run it with a debugger to see exactly where the crash occurs.
blades/combo_blade.h
Outdated
| bool Charging() override { return real_style_->Charging(); } | ||
|
|
||
| bool IsHandled(HandledFeature effect) override { | ||
| if (real_style_) |
There was a problem hiding this comment.
once UnsetStyle is called properly below, real_style_ will never be null here.
KeeganMorrow
force-pushed
the
combo_blade_pr
branch
from
c30ae69 to
3e227c1
Compare
| { | ||
| blade_ = blade1; | ||
| } | ||
|
|
There was a problem hiding this comment.
Needs a destructor that deletes the blade2_cache_
|
|
||
| BladeStyle* UnSetStyle() override { | ||
| BladeStyle* result = real_style_; | ||
| real_style_ = nullptr; |
There was a problem hiding this comment.
If you have a config file that crashes (and instructions for how to make it crash if needed) I can run it with a debugger to see exactly where the crash occurs.
Here is the combo blade code that I came up with. I admit that my current test case of combining the two crystal chamber LEDs is about as simple as it gets, but it seems to work pretty nicely for my use case.
The ComboBladeWrapper is used to send out calls to the the actual blades controlling real hardware. The style on the actual blades is set to a ComboBladeStyleWrapper, which forwards the calls for the on to the actual style code, but running against the original ComboBladeWrapper. This allows the ComboBladeWrapper to dispatch the led control calls on to the actual blade hardware while simulating a single blade with the length of both blades put together.
Also see discussion on crucible