[fp_conversions] Fix `str_to_fp` LSB always being zero

[LRitzdorf]

Previously, the value returned by the str_to_fp() function would always have an LSB of zero. Although the remaining bits are correctly aligned, this still resulted in an effective fractional precision one bit lower than expected.

This was discovered when attempting to write a value of 0.0625 (0x01, in UQ4.4 format) to a particular hardware register, which actually parsed to 0x00. Further testing revealed this "LSB equals zero" error to be very reliable — for instance, 0.9999 parsed to 0x0E instead of 0x0F, and 0.1875 to 0x02 instead of 0x03.

[LRitzdorf]

@tvannoy Tagging you as a potential reviewer for this, since you're credited in the relevant file header.

[tvannoy]

We typically don't show students these conversion functions or tell students to use these functions in their device drivers, so thanks for catching this error. Students usually just type in hex representations for the sysfs attributes.

We have used these functions before in a previous project, but never wrote any proper unit tests, so we never caught this bug.

I ran through your example of using 0.0625 using MATLAB's fixed-point toolbox, and the algorithm worked properly.

However, before we proceed with this PR, we should really write some unit tests so we can be sure that these functions work in all cases. I don't have time to write any unit tests right now. Using kunit seems probably too complicated to be worthwhile, even though we include some kernel headers (I think the only header we are actually using is <linux/types.h>). Perhaps Unity would work for us.

Additionally, as far as I can tell, there's no good reason to have that ARBITRARY_CUTOFF_LEN stuff in there. We can handle users typing in an arbitrarily long string; we'll just ignore any fractional digits that can't be represented in the given number of fractional bits (which is what we already do).

My point is that we should clean this code up while we're thinking about it. I think this code is poorly documented and difficult to follow.

@nat-sweeney @w-culhane @rksnider

[LRitzdorf]

This is obviously your guys' repo, so I don't want to get in your way, but I do have a few thoughts with regard to unit tests and their relevance.

• Unit tests are most useful in CI contexts, where code changes frequently. This feels like that shouldn't really be the case for this repo. Are the intermittent benefits of testing worth the overhead of creating those tests?
• What should be tested? There are lots of different languages in this repo, and the types of test harnesses they'd need would be quite varied, which complicates test writing and execution.
• Will tests confuse students using this repo? Granted, most of them will probably arrive via links from the textbook which direct them to specific files, and those files shouldn't contain test code. But, the presence of other files that they don't recognize might be confusing, especially if they try to consult those files for additional assignment context.

Also, should we move this discussion to an issue? It seems to be experiencing a fair bit of scope creep from the original PR.

[tvannoy]

What should be tested?

Just the functions in fp_conversion.h would be tested. As I said, we never wrote any tests for these; if we would have, we would've found this bug much sooner.

Certainly test code is most useful when

1. you are first writing the code (test driven development)
2. the underlying code changes frequently

Neither of those situations are true right now. However, I still prefer automated tests over manual testing because automated tests are reproducible and provide traceability that the code is actually correct. Writing software tests for this conversion code doesn't add much overhead; we don't even need to use a unit testing framework---a simple fp_conversion_test.c with a few functions would suffice. I think a simple unit test is preferable to needing a device driver, and possibly even an FPGA design with registers, just to test that the conversions work correctly. When a unit test will suffice, I'd prefer unit tests over integration tests.

But, the presence of other files that they don't recognize might be confusing, especially if they try to consult those files for additional assignment context.

Excuse me if I'm wrong, but I don't think fp_conversions.h is linked or mentioned anywhere in the textbook as far as I remember. If there indeed is no link or mention of fp_conversions.h in the textbook, and we never require using it, then fp_conversions.h is an extra file that might be confusing to students.

How did you find fp_conversions.h? I know it's commented out in the driver source code, so perhaps that is how. If it is in the textbook, then my bad---please point me to the relevant page if that's the case.

If it's not in the textbook, and we never use it, then perhaps we should simply remove the file. I agree that extra, unused files, can be confusing.

[LRitzdorf]

It was uploaded by @rksnider in 0894c4c379aacc325d9745556d707282b9042782 — not sure where he normally keeps it. It came up during a discussion after class, and is not linked in the textbook (that I know of).