ffmemless: Adding constant and custom periodic effects. Upload before play. #1
Conversation
| @@ -67,10 +67,9 @@ int ffmemless_erase_effect(int effect_id, int device_file) | |||
| } | |||
| } | |||
|
|
|||
| int ffmemless_evdev_file_open(const char *file_name) | |||
| int ffmemless_evdev_file_open(const char *file_name, unsigned long features[4]) | |||
There was a problem hiding this comment.
features are now tested in plugin.c
| copy->iface = iface; | ||
| copy->request = request; | ||
| copy->playback_time = data->playback_time; | ||
| memcpy(copy, data, sizeof(struct ffm_effect_data)); |
There was a problem hiding this comment.
this is also needed for the whole cached effect to be copied
src/plugins/ffmemless/plugin.c
Outdated
| int result = TRUE; | ||
|
|
||
| if (play) { | ||
| data->cached_effect.id = -1; |
There was a problem hiding this comment.
we reset the initial upload Id and re-upload it
src/plugins/ffmemless/plugin.c
Outdated
| result = FALSE; | ||
|
|
||
| if (!play) { | ||
| if (ffmemless_erase_effect(data->cached_effect.id, ffm.dev_file)) |
There was a problem hiding this comment.
very important to erase the effect at stop, since the device then runs out of 'space'
| // drivers which use FF_PERIODIC only with FF_CUSTOM waveform.. | ||
| // https://github.com/torvalds/linux/blob/master/drivers/input/ff-core.c#L350 | ||
| if (ffmemless_has_feature(FF_CONSTANT, ffm.features)) { | ||
| N_DEBUG (LOG_CAT "Using constant default effect, rumble is %d", |
There was a problem hiding this comment.
this is just to see if rumble is enabled too, in logs
src/ngf/main.c
Outdated
| @@ -179,7 +179,7 @@ main (int argc, char *argv[]) | |||
| AppData app; | |||
|
|
|||
| memset (&app, 0, sizeof (app)); | |||
| app.default_loglevel = N_LOG_LEVEL_ERROR; | |||
| app.default_loglevel = N_LOG_LEVEL_DEBUG; | |||
There was a problem hiding this comment.
You should not change the default log level.
There was a problem hiding this comment.
Thanks, changed. Let me know if you have suggestions for improvement for the other changes too. For example, flag in the config file for caching the effects, or detection of the rumble effect maybe should be fixed in my kernel etc.
|
Hey I tried your PR rebased on Xperia I (Sony-Kumano) with qti-haptics driver. The driver complains about this then: Check my ngfd log here: My rebase of this PR against master: |
|
Good to see one more person able to test this @Thaodan, thanks! old incorrect analysisSo you got a `data` with three `s16`: `effect`, `timeout_sec`, `timeout_msec`. This PR seems to only support two `s16` because `sizeof(int)` is still 32 on aarch. I need to add a different representation for CUSTOM_DATA, maybe a string that is interpreted as hex => so you also get the len as 'input'.It is still worth trying with this version (though it will read 1 more int outside the custom data buffer). Meanwhile, I see that these may be the effects: https://github.com/mer-hybris/android_kernel_sony_msm/blob/hybris-sony-aosp/LA.UM.7.1.r1/arch/arm64/boot/dts/qcom/pm8150b.dtsi#L501 Since you need to pass something like 196610 for effect 2, timeout 3. See this quick example: https://onlinegdb.com/quCtHIrzg Let me know if that works and how do you feel about adding Later edit: I just noticed that 1. you haven't actually passed 26144 as _CUSTOM (just opened the whole logs). And that sec and msec are not INPUT params. This may be something else |
|
Hi @Thaodan , I've made two changes: custom-data is now 8 bytes to match the 6 required, and it's allocated on heap. One of these was the reason for what seemed to be like memory corruption that you revealed. You can just cherry-pick this last commit. |
|
Hey I've tested your changes. After this change it works better except that after a time I hit; |
|
|
@Thaodan I've found the dbus reply regression, let me know if this last commit works better for you |
|
As already said: the PR works much better now except sometimes an effect gets erased twice, resulting in invallid argument of the second erase. Also the vibration effect is quite weak |
|
Thanks @Thaodan, here's my list of improvement incoming for this PR:
All in all, this will also set in stone the FF_CUSTOM behaviour to the one observed in our two implementations - which was not clear to me when I made the PR (I only had one data point).
Have you played with different magnitudes? On my device there's a setlist like |
If CACHE_EFFECTS is used. |
| @@ -585,7 +687,9 @@ static int ffm_sink_prepare(NSinkInterface *iface, NRequest *request) | |||
| * keep repeating them until timer runs out and effect ends. | |||
| */ | |||
| copy->repeat = INT32_MAX; /* repeat to "infinity" */ | |||
| copy->playback_time = playback_time; | |||
| if (!copy->playback_time && playback_time) { | |||
There was a problem hiding this comment.
this was resetting playback time to zero on CONSTANT effects, which was not nice.
|
I've re-worked the PR per the above 2 of the three points. |
|
Great work, thanks @b100dian :) I tested this on my Xperia 5 (another kumano) and for the most part it works great without One thing I noticed is that when using Waydroid and triggering some vibration from there, once back to SFOS the vibration "doesn't work" as if the vibrator gets "stolen" by the Android container (it's incredibly weak in SFOS as @Thaodan said in #1 (comment) and the weakness also happens when In the logs this is what I find yet don't know if the following is even related: The Any idea as to what might be going on? |
|
Thanks @voidanix, Regarding Waydroid - I do not experience this on Mi Note 101 Maybe in your case vibrator service and its access from Waydroid could just be disabled? Sorry if CACHE_EFFECTS doesn't make a difference (actually looks like you have the better driver;D) Maybe you can strace what the permission denied is about. I don't know what ausrv is, but you can get the PID of ngfd and Footnotes
|
|
Sorry for replying late here :P
As mentioned above,
Oops, I meant that the vibrator is weak when either:
I have not yet tried using both Waydroid and ngfd compiled with
The strace contentLooking at the thread you have given, modifying the host's I think this pretty much resolves to Waydroid not supporting the QTI haptics HAL here in use: its vibrator HAL points to nonexistent sysfs nodes and the host HAL uses Hope I didn't hijack this PR with useless stuff and thanks again :) |
|
BTW, I just wanted to add that with In dmesg when unlocking and moving a pulley up/down: journalctlIn case it ends up useful, you might want to check the vibrator HAL for the Xperia 1/5: https://github.com/sonyxperiadev/vendor-qcom-opensource-vibrator |
|
Thanks @voidanix,
This seems to be an error in the initialization of the custom data (this one https://github.com/mer-hybris/android_kernel_sony_msm/blob/hybris-sony-aosp/LA.UM.7.1.r1/drivers/input/misc/qti-haptics.c#L888 ) That field is parsed here, copied (in the case of CACHE_EFFECTS) here and used before upload is done here (and is meant to be like this https://onlinegdb.com/NRkfrRM4N ). As the author of the lines I am not able to find (am blind about) what gets wrong here. Can you help? Also I can make this somewhat switchable from the config file, but I don't like that the struct field is always present even if not used (see https://github.com/sailfishos-on-tucana/ngfd/tree/tucana/constant-optin ) |
|
@b100dian could it be that Doing some printf debugging around This number is now in the mid Looking at: You can clearly tell there is an overflow going on: arr.cpp:7:23: warning: overflow in conversion from ‘int’ to ‘int16_t’ {aka ‘short int’} changes value from ‘603380224’ to ‘-9728’ [-Woverflow]
7 | int16_t lol = 603380224;
| ^~~~~~~~~The re. config: it is the one you already provided for tucana, the values seem to match the ones used by my kernel dts/driver/HAL/whatever so it should be good. Will try your |
src/plugins/ffmemless/plugin.c
Outdated
| if (play) { | ||
| data->cached_effect.id = -1; | ||
| if (data->cached_effect.type == FF_PERIODIC) { | ||
| int16_t custom_data[CUSTOM_DATA_LEN] = {data->customEffectId}; | ||
| custom_data[0] = data->customEffectId; | ||
| data->cached_effect.u.periodic.custom_data = custom_data; |
There was a problem hiding this comment.
Does that store a pointer to a variable allocated on stack (which soon goes out of scope) in a dynamically allocated structure or am I missing something?
There was a problem hiding this comment.
Thanks @monich for taking a look at this PR!
Yes, for the sake of upload, it does that. The data should be copied to kernel by the driver.
More or less what's happening here https://github.com/sonyxperiadev/vendor-qcom-opensource-vibrator/blob/aosp/LA.UM.7.1.r1/Vibrator.cpp#L80
There was a problem hiding this comment.
Actually, I did miss that it doesn't go out of scope anymore, before being accessed. That's better))
There was a problem hiding this comment.
There was a problem hiding this comment.
I understand now what you're saying - the stuct was too localized inside the if to be available outside, where the actual upload happens. So it was not 'compiler optimizes it out' incorrectly, it was correctly discarding it (in a previous experiment I noticed that logging the values 'made' them correct). Thanks for restoring sanity!
|
I haven't added comments in a while, but what happened in the last 2 weeks was that @voidanix helped identify through logging that there was a compiler optimisation that prevented to local custom_data variable from being set at all when defined close to the upload, so I worked this around by moving its definition slightly in an outer scope. This should now work with CACHED_EFFECTS on Xperia 1/5 too, I hope. |
There was a problem hiding this comment.
Thanks for the PR! I would like to see the cache effects feature to be runtime configurable with defaulting to "false" to include this in the main tree.
It looks like the behavior doesn't change with undefined CACHE_EFFECTS.
There were also some unnecessary new lines added or removed which could be cleaned while doing other changes.
To configure this cache feature in adaptations which need this they can have file /usr/share/ngfd/plugins.d/60-ffmemless.ini with
[ffmemless]
cache_effects = true
Read from plugin props, something like:
cache_effects = !g_strcmp0(n_proplist_get_string(props, "cache_effects"), "true");
Thanks @jusa for reviewing this. The only drawback from runtime configuration is that the field cached_effect will be present for all, just not used.
Also thanks for acknowledging that the hardest part in this is getting a bool out of the config file:) One version I did before was taking into account only the key presence, but comparing to "true" sounds good enough (and better than that). |
|
Could you still squash the commits, prefix the commit subject line with |
Thanks, I will address this later today. Just to be clear, you would prefer a single commit or would two also do it? |
The custom support is observed in two implementatins as using 3 words for the data, only the first being input (id) and the other two being output playback time. Also adds effect caching so they can be uploaded right before play. [ffmemless] Added constant and custom periodic effects. Fixes JB#57639 [ffmemless] Enable effect caching and upload cached effects before play. JB#57639
b100dian
changed the title
There are two changes in this PR for this ffmemless driver: https://github.com/b100dian/Xiaomi_Kernel_OpenSource/blob/tucana-q-oss/drivers/input/misc/drv260x_input.c#L1143
First, it only supports Constant effect or periodic effect with Custom waveform (see the link above, where the if/else is).
So this implements support for that.
Second, the upload itself is mutating global state on the driver (see the link above) so you have to always update before play.
This is implemented as a hardcoded define, but I can migrate it to a config setting.
Here's an example configuration with constant and custom periodic effect: https://github.com/sailfishos-on-tucana/droid-config-tucana/blob/master/sparse/usr/share/ngfd/plugins.d/51-ffmemless.ini#L57