dji-sdk / Mobile-SDK-Android

DJI Mobile SDK for Android: http://developer.dji.com/mobile-sdk/
Other
983 stars 579 forks source link

BatteryState: Product vs. Remote Controller #598

Open OlivierMartineau opened 4 years ago

OlivierMartineau commented 4 years ago

In SDK 4.13 I was surprised to see that Remote Controller "ChargeRemaining" class was changed into "dji.common.remotecontroller.BatteryState". Indeed, "BatteryState" was already a class that makes it possible to get the product battery state (dji.common.battery.BatteryState).

This is a wrong design.

First, one cannot import both classes in the same file.

import dji.common.battery.BatteryState;
import dji.common.remotecontroller.BatteryState; // Not accepted by the compiler as is, needs aliasing

Besides, there are similar methods that don't even have the same name: dji.common.remotecontroller.BatteryState.getRemainingChargeInPercent() vs. dji.common.battery.BatteryState.getChargeRemainingInPercent()

This is very confusing and calling for mistakes.

A better design would be to have two classes: ProductBatteryState and RemoteControllerBatteryState for instance, that both inherit from a generic BatteryState class with (possibly abstract) homogenized common methods.

dji-dev commented 4 years ago

Agent comment from William Wong in Zendesk ticket #38560:

Dear Client Thank you for contacting DJI.

I have got your idea. I will report this situation to the dev team and let them decide what to do next.

Hopefully our solution can help you. Kindly Regards, DJI Developer Support

OlivierMartineau commented 4 years ago

Thank you for submitting a ticket, looking forward to hearing from you!

dji-dev commented 4 years ago

Agent comment from William Wong in Zendesk ticket #38560:

Dear Client Thank you for contacting DJI.

I am sorry that I have been informed that this issue will not be modified in 4.14 version since this can be fixed in code and we are short in hands. Everyone is busy on fixing some urgent issues. This issue can be fixed by coding like this. You can only import 1 package or don't import package. Then you need to specify where the batteryState comes from when you are using it. ​

Hopefully our solution can help you. Kindly Regards, DJI Developer Support
90B73339-A8B0-AE2D-E2FB-6A884CE0EFAF.png

OlivierMartineau commented 4 years ago

Hello,

Thank you for your answer.

For now, indeed, I have to explicitly specify in my code the packages each "BatteryState" class is referring to.

updateComponentState(dji.common.battery.BatteryState aircraftBatteryState) updateComponentState(dji.common.remotecontroller.BatteryState remoteControllerBatteryState)

It works, but it's uncomfortable and is likely to introduce incomprehension and errors for SDK users. I do hope this is now somewhere on the roadmap of bugs to be addressed in a not so distant future. As I said earlier, the most elegant solution would be to unify both classes under a common BatteryState interface, and have a ProductBatteryState and RemoteControlBatteryState. This would make sense, and make it possible to have common methods in both classes.

I wish you the best! Your products are amazing, truly. Keep up the good work!

Olivier MARTINEAU CEO – EOLE EYES SAS +33 6 61 92 30 67 www.eole-eyes.com

Le mar. 15 sept. 2020 à 08:40, DJI notifications@github.com a écrit :

Agent comment from William Wong in Zendesk ticket #38560 https://djisdksupport.zendesk.com/agent/tickets/38560:

Dear Client Thank you for contacting DJI.

I am sorry that I have been informed that this issue will not be modified in 4.14 version since this can be fixed in code and we are short in hands. Everyone is busy on fixing some urgent issues. This issue can be fixed by coding like this. You can only import 1 package or don't import package. Then you need to specify where the batteryState comes from when you are using it.

https://camo.githubusercontent.com/81e0ab536969317e140ea46177f7ca5325b9e9e6/68747470733a2f2f646a6973646b737570706f72742e7a656e6465736b2e636f6d2f6174746163686d656e74732f746f6b656e2f744e4259315059574274746b63536650334a736a486c6e47332f3f6e616d653d39304237333333392d413842302d414532442d453246422d3641383834434530454641462e706e67

Hopefully our solution can help you. Kindly Regards, DJI Developer Support 90B73339-A8B0-AE2D-E2FB-6A884CE0EFAF.png https://djisdksupport.zendesk.com/attachments/token/tNBY1PYWBttkcSfP3JsjHlnG3

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dji-sdk/Mobile-SDK-Android/issues/598#issuecomment-692500874, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKRXN75KOMO3IYZUUWIX3ELSF4D4BANCNFSM4QMU6EZA .