AUTOCALIBRATE command for Sniff-ISO15693 application

[cacke-r]

This adds code to the Sniff15693 Application to run the AUTOCALIBRATE command. But default, the Sniff15693 codec uses an Autothreshold detection - if this works fine, it shall be used. If not you can disable it by un-commenting this line in the Makefile:

CONFIG_SETTINGS += -DSNIFF15693_DISABLE_AUTO_THRESHOLD

The AUTOCALIBRATION algorithm itself is explained in the source code (Application/Sniff15693.c)

[cacke-r]

@ceres-c @fptrs pls find the PR here. Looking forward for your review comments. (I*ll be on vacation for few days - so i'll work on the findings only b/o next week)

[ceres-c]

I've got a question: if we're going for compile time #ifdefs, why having the disable_autothreshold boolean variable, if it is read only? If compile time switches are the way to go, everything could be put in preprocessor branches and only one of the two versions would be compiled.

TBH I believe that compile time switches are not the way to go. I think it'd be easier for users (albeit experienced users) to understand how to use the auto threshold for ISO15 if it was actually possible to use the command in a normal way. If I was the user and I had not followed this discussion I'd find this behavior unexpected and quite confusing

[cacke-r]

@ceres-c I think I got your point - espacially the 'how to make others aware in how to use it' I'm happy to add a command here - but I'm not fully convinced if it solves the problem.

After reading your comment - I though about it - and I think the real dilemma we are in is "We have 2 commands/parameters/#ifdefs which are somehow independent of each other ... and somehow not :-(" To make it clear:

• the 'always switch off the autothreshold of the codec
• run the autocalibrate command

I've got a question: if we're going for compile time #ifdefs, why having the disable_autothreshold boolean variable, if it is read only? If compile time switches are the way to go, everything could be put in preprocessor branches and only one of the two versions would be compiled.

While implementing, I had the goal to keep both independent. Means - even if someone whats to go with autothreshold - it should be possible to run the autocalibrate command (maybe, just to find out how big the threshold is) ..... this means, during the command execution - we need to switch off autothreshold

TBH I believe that compile time switches are not the way to go. I think it'd be easier for users (albeit experienced users) to understand how to use the auto threshold for ISO15 if it was actually possible to use the command in a normal way. If I was the user and I had not followed this discussion I'd find this behavior unexpected and quite confusing

OK, makes sense - but will not completely solve the dilemma described above. What do you think of the follwoing approach:

• implement command to 'disable autothreshold'
• implement command to 'autocalibrate'
• 'autocalibrate' will throw an error if the 'disable autothreshold' was not executed bofore
  • I'd say that is more transparent to the user - the alternative would be to en-/dis-abling it automatically - but I feel this would cause even more confusion.

I would appreciate if you have any advise if the approach above makes sense for you.

[cacke-r]

@ceres-c @fptrs The PR was updated automatically. I've added the command - but I'm not yet done. I need to rework the other patches as well. Will do this tomorrow and send you a ping here, once I'm done. If you want you can already review the 2 new patches - they are ready. It is just the rest which needs rework now.

[ceres-c]

Hi, sorry for the delay, I like this approach, it now sounds user-friendly enough to me :)

[cacke-r]

@ceres-c @fptrs so, finally I've added a command to control the autothreshold feature of the codec and removed the ifdefs. I needed to re-arrange the patches a bit. Looking forward for further feedback ... or a merge :-)

[cacke-r]

@ceres-c @fptrs Hey guys, I don't want to appear pushy 😀 but could you let me know I which timeframe I could expect further feedback ?

[fptrs]

Hi @cacke-r, sorry for the late reply. I'm going to test/review it this week. If everything is fine I'm going to merge it.

[fptrs]

So I tested the autocalibration and it works fine. Very good job :+1: There is only one final request before I'm going to merge it. Can you change the autothreshold command to accept 1 and 0 as parameters instead of strings, similar to the field command.

[cacke-r]

So I tested the autocalibration and it works fine. Very good job +1 There is only one final request before I'm going to merge it. Can you change the autothreshold command to accept 1 and 0 as parameters instead of strings, similar to the field command.

@fptrs thanks for testing and reviewing this. I've just pushed the changes about ENABLE/DISABLE vs 0,1.

Yesterday I realized that there is still one issue with this autocalibrate. If we never receive a valid frame - it will set the threshold to (0+0xFFF/2) - which is not so nice. I'll work to fix it - means detect this as an error case - and set the threshold to the value it was before.

I'd like to raise this as a new PR - just to not open up this one again. But if you prefer to have it as part of this PR - pls let me know.

[cacke-r]

So I tested the autocalibration and it works fine. Very good job +1 There is only one final request before I'm going to merge it. Can you change the autothreshold command to accept 1 and 0 as parameters instead of strings, similar to the field command.

@fptrs thanks for testing and reviewing this. I've just pushed the changes about ENABLE/DISABLE vs 0,1.

Yesterday I realized that there is still one issue with this autocalibrate. If we never receive a valid frame - it will set the threshold to (0+0xFFF/2) - which is not so nice. I'll work to fix it - means detect this as an error case - and set the threshold to the value it was before.

I'd like to raise this as a new PR - just to not open up this one again. But if you prefer to have it as part of this PR - pls let me know.

I've pushed the fix to a branch in my repo - I'd leave it up to you if I should include it here: https://github.com/cacke-r/ChameleonMini/commit/c47974e44d5f7f6c4d3ab2672c4f20acf5f9d722

[fptrs]

I think the best solution is to include it here directly

[cacke-r]

I think the best solution is to include it here directly

OK, thanks for the feedback. Included the fix as well. So - I think I'm done now :-)