vibrator: rework effects and fix bugs

Rework the EV_FF interfacing to avoid abusing the API, the new
implementation builds the effects during initialisation and uploads them
all, then when playing an effect we look up the id of the effect to play
and send the play command.

Then when off() is called we send a stop command for that effect ID.
This implementation lends itself much better to the EV_FF API and can
easily be expanded to support newer HAL versions with richer effects.

Sometimes seeing: "E VibratorService: Failed to issue command to
vibrator HAL. Retrying." but not sure what causes it.
diff --git a/vibrator/Vibrator.cpp b/vibrator/Vibrator.cpp
index 3d1b413..f77cfd4 100644
--- a/vibrator/Vibrator.cpp
+++ b/vibrator/Vibrator.cpp
@@ -46,156 +46,198 @@
 using EffectStrength = ::android::hardware::vibrator::V1_0::EffectStrength;
 using Effect = ::android::hardware::vibrator::V1_1::Effect_1_1;
 
-static constexpr int8_t MAX_TRIGGER_LATENCY_MS = 6;
+// These have to be kept in line with HAL version
+static constexpr Effect MAX_EFFECT = Effect::TICK;
+static constexpr EffectStrength MAX_EFFECT_STRENGTH = EffectStrength::STRONG;
+
+#define EFFECT_INDEX(effect, strength) \
+	((uint16_t)effect * (uint16_t)MAX_EFFECT_STRENGTH + (uint16_t)strength)
 
 Vibrator::Vibrator(std::string devpath) :
-    mInputDevPath(devpath)
+	mInputDevPath(devpath)
 {
-    std::memset(&mEffect, 0, sizeof(mEffect));
-    mEffect.type = FF_RUMBLE;
-    mEffect.id = -1;
+	// We just keep the input device open the whole time we're running.
+	// Closing / reopening it seems to break things.
+	mfd = openInputDev();
+	if (mfd < 0) {
+		ALOGE("%s() can't open FF input device %s", __func__, mInputDevPath.c_str());
+		return;
+	}
 
-    mfd = openInputDev();
-    if (mfd < 0) {
-		ALOGE("CA:: %s() can't open FF input device %s", __func__, mInputDevPath.c_str());
-        return;
+	// Build a table of effects for each strength of each effect.
+	int baseStrength = 0;
+	for (uint16_t i = 0; i <= (uint16_t)MAX_EFFECT; i++)
+	{
+		for (uint16_t j = 0; j <= (uint16_t)MAX_EFFECT_STRENGTH; j++)
+		{
+			switch((Effect)i) {
+				default:
+				case Effect::CLICK:
+					baseStrength = 0x4000;
+					break;
+				case Effect::TICK:
+					baseStrength = 0x1000;
+					break;
+				case Effect::DOUBLE_CLICK:
+					baseStrength = 0x3000;
+					break;
+			}
+			ALOGV("%s() uploading effect %d, strength %d, magnitude = 0x%x", __func__, i, j, baseStrength + j * 0x1000);
+			// 0x8000 is about the max for qcom_spmi_haptics.
+			mEffects[EFFECT_INDEX(i, j)] = FF_EFFECT((uint16_t)(baseStrength + j * 0x800));
+
+			uploadEffectToKernel(&mEffects[EFFECT_INDEX(i, j)]);
+		}
 	}
 }
 
 int Vibrator::openInputDev() {
-    return open(mInputDevPath.c_str(), O_RDWR|O_CLOEXEC);
+	return open(mInputDevPath.c_str(), O_RDWR|O_CLOEXEC);
 }
 
-Return<Status> Vibrator::on(uint32_t timeoutMs, uint32_t playCount) {
-    ALOGI("CA:: %s, timeoutMs = %d, playCount = %d, effect.id = %d, strong magnitude = %d, weak magnitude = %d", __func__, timeoutMs, playCount, mEffect.id, mEffect.u.rumble.strong_magnitude, mEffect.u.rumble.weak_magnitude);
-    struct input_event play;
-    int ret;
-
-    // Upload Rumble effect
-    ret = ioctl(mfd, EVIOCSFF, &mEffect);
-    if (ret < 0) {
-        ALOGE("CA:: %s() Couldn't upload rumble effect to device %s, ret = %d", __func__, mInputDevPath.c_str(), ret);
-        return Status::UNKNOWN_ERROR;
+void Vibrator::uploadEffectToKernel(struct ff_effect* effect) {
+	int ret;
+	
+	ret = ioctl(mfd, EVIOCSFF, effect);
+	if (ret < 0) {
+		ALOGE("%s() Couldn't upload rumble effect to device %s, ret = %d", __func__, mInputDevPath.c_str(), ret);
 	}
+}
 
-    play.type = EV_FF;
-    play.code = mEffect.id;
-    play.value = playCount;
-    ret = write(mfd, (const void*) &play, sizeof(play));
-    if (ret != sizeof(play)) {
-        ALOGE("CA:: %s() failed to fully write play event to input device: %d", __func__, errno);
-        return Status::UNKNOWN_ERROR;
-    }
-
-    usleep(timeoutMs * 1000);
-    
-    return Status::OK;
+void Vibrator::deleteEffectFromKernel(struct ff_effect* effect) { // Unload rumble effect
+	int ret;
+	
+	ret = ioctl(mfd, EVIOCRMFF, effect);
+	if (ret < 0) {
+		ALOGE("%s() Failed to remove rumble effect from device %s, ret = %d", __func__, mInputDevPath.c_str(), ret);
+		return;
+	}
+	effect->id = -1;
 }
 
 // Methods from ::android::hardware::vibrator::V1_1::IVibrator follow.
 Return<Status> Vibrator::on(uint32_t timeoutMs) {
-    return(on(timeoutMs, 1));
+	struct ff_effect* effect;
+	// If the active effect is set, use it instead of the default
+	if (mActiveEffectId < 0) {
+		effect = &mEffects[EFFECT_INDEX(Effect::CLICK , EffectStrength::MEDIUM)];
+	} else {
+		effect = &mEffects[mActiveEffectId];
+	}
+
+	ALOGV("%s, timeoutMs = %d, effect.id = %d, magnitude = %d", __func__, timeoutMs, effect->id, effect->u.rumble.strong_magnitude);
+	struct input_event play;
+	int ret;
+
+	play.type = EV_FF;
+	play.code = effect->id;
+	play.value = 1;
+
+	ret = write(mfd, (const void*) &play, sizeof(play));
+	if (ret != sizeof(play)) {
+		ALOGE("%s() failed to fully write play event to input device: %d", __func__, errno);
+		return Status::UNKNOWN_ERROR;
+	}
+
+	usleep(timeoutMs * 1000);
+
+	return Status::OK;
 }
 
 Return<Status> Vibrator::off()  {
-    ALOGI("CA:: %s", __func__);
-    struct input_event stop;
-    int ret;
-
-    stop.type = EV_FF;
-    stop.code = mEffect.id;
-    stop.value = 0;
-    if (write(mfd, (const void*) &stop, sizeof(stop)) != sizeof(stop)) {
-        ALOGE("CA:: %s() failed to fully write stop event to input device", __func__);
-        return Status::UNKNOWN_ERROR;
-    }
-
-    // Unload rumble effect
-    ret = ioctl(mfd, EVIOCRMFF, &mEffect);
-    if (ret < 0) {
-		ALOGE("CA:: %s() Failed to remove rumble effect from device %s, ret = %d", __func__, mInputDevPath.c_str(), ret);
-        return Status::UNKNOWN_ERROR;
+	ALOGV("%s", __func__);
+	struct input_event stop;
+	struct ff_effect* effect;
+	// If the active effect is set, use it instead of the default
+	if (mActiveEffectId < 0) {
+		effect = &mEffects[(uint16_t)Effect::CLICK * (uint16_t)EffectStrength::STRONG];
+	} else {
+		effect = &mEffects[mActiveEffectId];
 	}
-    mEffect.id = -1;
 
-    return Status::OK;
+	stop.type = EV_FF;
+	stop.code = effect->id;
+	stop.value = 0;
+	if (write(mfd, (const void*) &stop, sizeof(stop)) != sizeof(stop)) {
+		ALOGE("%s() failed to fully write stop event to input device", __func__);
+		return Status::UNKNOWN_ERROR;
+	}
+
+	return Status::OK;
 }
 
 Return<bool> Vibrator::supportsAmplitudeControl()  {
-    ALOGI("CA:: %s", __func__);
-    return false;
+	ALOGV("%s", __func__);
+	return false;
 }
 
 Return<Status> Vibrator::setAmplitude(uint8_t amplitude) {
-    ALOGI("CA:: %s", __func__);
+	ALOGV("%s", __func__);
 
-    if (!amplitude) {
-        return Status::BAD_VALUE;
-    }
+	if (!amplitude) {
+		return Status::BAD_VALUE;
+	}
 
-    return Status::OK;
+	return Status::OK;
 }
 
 Return<void> Vibrator::perform(V1_0::Effect effect, EffectStrength strength,
-        perform_cb _hidl_cb) {
-    return performWrapper(effect, strength, _hidl_cb);
+		perform_cb _hidl_cb) {
+	return performWrapper(effect, strength, _hidl_cb);
 }
 
 Return<void> Vibrator::perform_1_1(V1_1::Effect_1_1 effect, EffectStrength strength,
-        perform_cb _hidl_cb) {
-    return performWrapper(effect, strength, _hidl_cb);
+		perform_cb _hidl_cb) {
+	return performWrapper(effect, strength, _hidl_cb);
 }
 
 template <typename T>
 Return<void> Vibrator::performWrapper(T effect, EffectStrength strength, perform_cb _hidl_cb) {
-    ALOGI("CA:: %s, ", __func__);
-    auto validRange = hidl_enum_range<T>();
-    if (effect < *validRange.begin() || effect > *std::prev(validRange.end())) {
-        _hidl_cb(Status::UNSUPPORTED_OPERATION, 0);
-        return Void();
-    }
-    return performEffect(static_cast<Effect>(effect), strength, _hidl_cb);
+	ALOGV("%s, ", __func__);
+	auto validRange = hidl_enum_range<T>();
+	if (effect < *validRange.begin() || effect > *std::prev(validRange.end())) {
+		_hidl_cb(Status::UNSUPPORTED_OPERATION, 0);
+		return Void();
+	}
+	return performEffect(static_cast<Effect>(effect), strength, _hidl_cb);
 }
 
 Return<void> Vibrator::performEffect(Effect effect, EffectStrength strength,
-        perform_cb _hidl_cb) {
-    Status status = Status::OK;
-    uint32_t timeMs;
-    int playCount = 1;
-    mEffect.u.rumble.weak_magnitude = mEffect.u.rumble.strong_magnitude = 0;
+		perform_cb _hidl_cb) {
+	Status status = Status::OK;
+	uint32_t timeMs = 9;
+	bool doubleClick = effect == Effect::DOUBLE_CLICK;
 
-    ALOGI("CA:: %s() effect = %d", __func__, effect);
+	ALOGV("%s() effect = %d, strength = %d", __func__, effect, (int)strength);
 
-    switch (effect) {
-    case Effect::TICK:
-        mEffect.u.rumble.weak_magnitude = 0x5000 + (int)strength * 0x1000;
-        timeMs = 9;
-        break;
-    case Effect::CLICK:
-        mEffect.u.rumble.strong_magnitude = 0x500 + (int)strength * 0x1000;
-        timeMs = 9;
-        break;
-    case Effect::DOUBLE_CLICK:
-        mEffect.u.rumble.strong_magnitude = 0x4000 + (int)strength * 0x1000;
-        playCount = 2;
-        timeMs = 130;
-        break;
-    default:
-        _hidl_cb(Status::UNSUPPORTED_OPERATION, 0);
-        return Void();
-    }
+	if (effect > Effect::TICK){
+		_hidl_cb(Status::UNSUPPORTED_OPERATION, 0);
+		return Void();
+	}
 
-    timeMs += MAX_TRIGGER_LATENCY_MS; // Add expected cold-start latency
+	mActiveEffectId = EFFECT_INDEX(effect, strength);
 
-    on(timeMs, playCount);
-    // Android calls off() for us.
-    _hidl_cb(status, timeMs);
+	// Play the effect
+	on(timeMs);
+	/*
+	 * Android calls off() for us, so
+	 * only call it if we're doing a double click
+	 */
+	if (doubleClick) {
+		off();
+		timeMs += 59;
+		usleep(50 * 1000);
+		on(timeMs);
+		mActiveEffectId = -1; // Reset the active effectId
+	}
 
-    return Void();
+	_hidl_cb(status, timeMs);
+
+	return Void();
 }
 
 Vibrator::~Vibrator() {
-    close(mfd);
+	close(mfd);
 }
 
 
diff --git a/vibrator/Vibrator.h b/vibrator/Vibrator.h
index 308f4fe..8e6140d 100644
--- a/vibrator/Vibrator.h
+++ b/vibrator/Vibrator.h
@@ -22,6 +22,7 @@
 #include <linux/input.h>
 
 #include <fstream>
+#include <map>
 
 // Borrowed from linuxconsole/utils/bitmaskros.h
 /* Number of bits for 1 unsigned char */
@@ -35,6 +36,26 @@
 /* Test the bit with given index=offset in an unsigned char array */
 #define testBit(bit, array)    ((array[ucharIndexForBit(bit)] >> bitOffsetInUchar(bit)) & 1)
 
+/*
+ * Builder for ff_effect struct.
+ * We don't bother using weak_magnitude here as most hardware
+ * doesn't have any way to differentiate strong / weak haptics
+  */
+#define FF_EFFECT(rumbleStrenth) 						\
+	{													\
+		.type = FF_RUMBLE, 								\
+		.id = -1, 										\
+		.direction = 0, 								\
+		.trigger = { .button = 0, .interval = 0, },		\
+		.replay = { .length = 0, .delay = 0, },			\
+		.u = {											\
+			.rumble = {									\
+				.strong_magnitude = rumbleStrenth,		\
+				.weak_magnitude = 0,					\
+			},											\
+		},												\
+	}
+
 namespace android {
 namespace hardware {
 namespace vibrator {
@@ -43,31 +64,34 @@
 
 class Vibrator : public IVibrator {
 public:
-    Vibrator(std::string mInputDevPath);
+	Vibrator(std::string mInputDevPath);
 
-    // Methods from ::android::hardware::vibrator::V1_0::IVibrator follow.
-    using Status = ::android::hardware::vibrator::V1_0::Status;
-    Return<Status> on(uint32_t timeoutMs)  override;
-    Return<Status> off()  override;
-    Return<bool> supportsAmplitudeControl() override;
-    Return<Status> setAmplitude(uint8_t amplitude) override;
+	// Methods from ::android::hardware::vibrator::V1_0::IVibrator follow.
+	using Status = ::android::hardware::vibrator::V1_0::Status;
+	Return<Status> on(uint32_t timeoutMs)  override;
+	Return<Status> off()  override;
+	Return<bool> supportsAmplitudeControl() override;
+	Return<Status> setAmplitude(uint8_t amplitude) override;
 
-    using EffectStrength = ::android::hardware::vibrator::V1_0::EffectStrength;
-    Return<void> perform(V1_0::Effect effect, EffectStrength strength, perform_cb _hidl_cb)
-            override;
-    Return<void> perform_1_1(Effect_1_1 effect, EffectStrength strength, perform_cb _hidl_cb)
-            override;
+	using EffectStrength = ::android::hardware::vibrator::V1_0::EffectStrength;
+	Return<void> perform(V1_0::Effect effect, EffectStrength strength, perform_cb _hidl_cb)
+			override;
+	Return<void> perform_1_1(Effect_1_1 effect, EffectStrength strength, perform_cb _hidl_cb)
+			override;
 
 private:
-    ~Vibrator();
-    Return<Status> on(uint32_t timeoutMs, uint32_t playCount);
-    int openInputDev();
-    template <typename T>
-    Return<void> performWrapper(T effect, EffectStrength strength, perform_cb _hidl_cb);
-    Return<void> performEffect(Effect_1_1 effect, EffectStrength strength, perform_cb _hidl_cb);
-    std::string mInputDevPath;
-    int mfd;
-    struct ff_effect mEffect;
+	~Vibrator();
+	void uploadEffectToKernel(struct ff_effect*);
+	void deleteEffectFromKernel(struct ff_effect*);
+	int openInputDev();
+	template <typename T>
+	Return<void> performWrapper(T effect, EffectStrength strength, perform_cb _hidl_cb);
+	Return<void> performEffect(Effect_1_1 effect, EffectStrength strength, perform_cb _hidl_cb);
+	std::string mInputDevPath;
+	int mfd;
+	int mActiveEffectId;
+	// Look up table of effects by type and strength
+	std::map<int, struct ff_effect> mEffects;
 
 };
 
@@ -78,17 +102,17 @@
 }  // namespace android
 
 class FileDescGuard {
-    public:
-        FileDescGuard(int fd) : m_fd(fd) {}
+	public:
+		FileDescGuard(int fd) : m_fd(fd) {}
 
-        ~FileDescGuard() {
-            if (close(m_fd) != 0)
-            {
-                ALOGE("CA:: Failed to close fd %d, errno = %d", m_fd, errno);
-            }
-        }
-    private:
-        int m_fd = -1;
+		~FileDescGuard() {
+			if (close(m_fd) != 0)
+			{
+				ALOGE("CA:: Failed to close fd %d, errno = %d", m_fd, errno);
+			}
+		}
+	private:
+		int m_fd = -1;
 };
 
 #endif  // ANDROID_HARDWARE_VIBRATOR_V1_1_VIBRATOR_H
diff --git a/vibrator/service.cpp b/vibrator/service.cpp
index 6d13730..2a05bc2 100644
--- a/vibrator/service.cpp
+++ b/vibrator/service.cpp
@@ -48,10 +48,10 @@
 	int request = EVIOCGBIT(EV_FF, sizeof(features)*sizeof(unsigned char));
 	bool supported = false;
 
-    ALOGE("CA:: %s: Testing device: %s", __func__, devname.c_str());
+    ALOGE("%s: Testing device: %s", __func__, devname.c_str());
 
 	if ((ret = ioctl(tempFd, request, &features)) < 0) {
-        ALOGE("CA:: %s: ioctl() failed with errno = %d", __func__, ret);
+        ALOGE("%s: ioctl() failed with errno = %d", __func__, ret);
 		supported = false;
     }
 	if (testBit(FF_RUMBLE, features)) {
@@ -83,7 +83,7 @@
         std::string devpath(devname);
         if (inputDevSupportsFF(devpath)) {
             closedir(dir);
-            ALOGI("CA:: %s(): Found haptics device: %s", __func__, devpath.c_str());
+            ALOGV("%s(): Found haptics device: %s", __func__, devpath.c_str());
             return devpath;
         }
     }
@@ -97,7 +97,6 @@
     if (hapticsDev.length() < 2)
         return UNKNOWN_ERROR;
     sp<IVibrator> vibrator = new Vibrator(hapticsDev);
-    ALOGI("CA:: %s", __func__);
 
     return vibrator->registerAsService();
 }