Additional registers and register documentation, and round tach output to 2dp

[rivimey]

I'm working on a project using 4 EMC2101's, an FT232H and a mux. I wanted more control and more status output from the EMC, so I have added register addresses for all, and variable definitions for most, of the device registers.

Three other changes:

• Rounding the tachometer output prevents values like 82.3582941144 (not really helpful and for most motors the reading jitters quite rapidly).
• Also, defined constants for the MFG, Part numbers and used them appropriately.
• Finally, added property to return the mfg, part and die revision.

I may work on this further. Let me know if there are things I need to know.

[rivimey]

I can see there was a problem with formatting, but not what the problem was. I may need help (or pointers) fixing it

[ladyada]

hiya! thanks so much for submitting a PR! we can review & merge PRs once they have passed continuous integration (CI). that means that code is 'linted' - that means it is formatted and passes basic structure tests, so that we maintain the same text formatting for all new code if your code isnt passing, check the CI output (click on the red X next to the PR to scroll through the log and find where the error is

here is a tutorial on pylint and black: https://learn.adafruit.com/improve-your-code-with-pylint

and overall how to contribute PRs to circuitpython: https://learn.adafruit.com/contribute-to-circuitpython-with-git-and-github

once you get that green checkmark that indicates CI has passed, please comment reply to this post so we know its time for another review (we may not get notified on CI pass and miss that its time to look!)

[rivimey]

Good to review now?

[tekktrik]

Also, thanks for adding all the registers, what a behemoth of a task!

[tekktrik]

My only other concern is memory usage. Are the new registers accessed by the library? Adding this many adafruit_register objects may make it unusable by smaller boards, just something to keep in mind. Not a dealbreaker, per se. What board are you using this with?

[rivimey]

As for memory usage I have no idea what acceptable limits might be or how to measure what the values are. Part of my reason for adding the new registers was to enable additional properties to read or write them, but have not done so in this pr just to get the ball rolling.

If memory use is really a concern, then at least some registers could be added instead to the LUT class, so those using the non-LUT class can go for the low-memory option that way. If you're very restricted for memory it seems unlikely you would be using the LUT (with its need for an in-memory version of the LUT).

My own usage is on a amd64 server with 64GB ram, so I don't care :-)

[tekktrik]

Okay, I was think more about this, and reading back on the PR that added the LUT class, I think a lot of this (particularly the heavy use of adafruit_register objects should go in that subclass for memory reasons, since I agree with you that using it probably indicates memory isn't a concern.

Wanna try keeping the "essential" functionality additions/changes as is, and adding more of the "extra" ones to the LUT class?

[rivimey]

I have moved the registers around a bit, and I hope things are looking better now.

[rivimey]

Hopefully this is in a good state now. I've tested it and it seems to work fine in my app at least.

Can I bring your attention to https://github.com/adafruit/Adafruit_Blinka/issues/578 which is related?

[tekktrik]

Thanks for all the edits! I'll take a deeper look at this very soon when I get a chance, but the cursory look I had is good!

[rivimey]

Bump?

[tekktrik]

Sorry, I haven't had a chance to get to this just yet but I haven't forgotten! I did mean to ask about the decision to make a new EXT class, is it worth separating from the LUT class in implementation? Not against it, just noticed it!

[rivimey]

I'll answer some comments inline as well, but this set of changes includes some not quite complete things, such as the blank comments. That's why it seems a little off. Sorry!

[rivimey]

As I indicated earlier, OSError makes no sense to me when the exception description is "This exception is raised when a system function returns a system-related error, including I/O failures such as “file not found” or “disk full” (not for illegal argument types or other incidental errors)."

The problem (assuming there is one) is a hardware one, probably that an I2C cable has been unplugged or the device has entered a bad state.

[tekktrik]

It indicates an I/O error. In CircuitPython, it gets raised when things like a file can't be accessed, or I2C communication failure occurs. If those would be a physical hardware problem resulting in I2C error or something else about I2C going wrong, that is why raising them as OSError makes the most sense.

Here are a couple examples of OSError, both of being raised and caught:

• https://github.com/adafruit/Adafruit_CircuitPython_BusDevice/blob/318f7f79292ab6f0e24b2c6f8964c42d0e43a7a2/adafruit_bus_device/i2c_device.py
• https://github.com/adafruit/Adafruit_CircuitPython_FeatherWing/blob/08e2843a03d5324bcc75e49a686054bcac4b0dda/examples/featherwing_tft24_simpletest.py

[FoamyGuy]

I think it's worth having a discussion with the wider community and contributors to talk through whether we want it to check the status register and raise exceptions from there.

I can see the upside of having the code crash as soon as it knows something is wrong (due to the status register saying so). But I do notice it makes the current examples unable to run with the new version using the same testing setup that they do work with using the released version of the driver.

For example, both the emc2101_set_pwm_freq.py and [emc2101_lut_example.py](https://github.com/adafruit/Adafruit_CircuitPython_EMC2101/blob/main/examples/emc2101_lut_example.py) examples won't run for me using the new version because the LUT version of the driver class has the check_status calls that end up raising exceptions.

Both do work normally using the same hardware and circuit with the current driver.

[rivimey]

The device signals various exception states in the status register, and if configured, can raise an interrupt on the Alert line when status changes. However, gaining the interrupt request line means losing the tach input line.

Until this recent change the status register was ignored. I put in this check to pass on such signals, which include both open/short circuit faults, temperature and fan speed limits exceeded (lower and upper). Without such checks there is essentially no point in setting the tach or temp limit values.

I understand we might not want this all the time, which is why in the EMC2101_EXT there is a way to disable it (set auto_check_status to False). For the base class though I didn't include that mechanism for space constraint reasons. It could easily be done.

One way to massively reduce memory requirements would be to remove the classes based on "CV", which take up at least 1KB of bytecode and 3KB of native code. It would, of course, be a breaking change.

I would emphasise that the status register checks are not 'something is wrong with the code or the program, but that the hardware monitor is doing its job and telling you when the fan is too fast, too slow or the temp is too high or low for the situation. I fully agree that 'crashing' from an exception is not the right response. However, completely ignoring the status register isn't useful either.

[rivimey]

I have just pushed some more exception handling changes as requested. I think if this PR is accepted there will need to be release notes and additional docs because there are some breaking changes (different exceptions) as well as new features.

I need to wind down work on this for other things very soon as other things are calling on my time.

I am no longer sure where this review has got to. Could (both) of you explicitly note any changes still needed and make a list in a new comment? I think the point with check_status() exceptions needs to be pinned down. One option might be to remove check_status from the EMC2101 class and move those calls to an override of the methods in the EMC2101_EXT class -- so the base class doesn't check by default but the EXT class can do so if you ask it to. Either way, should the default be to check or not?

[tekktrik]

I'm happy to do a new review on the code to get a list what's left, not taking into account any changes with regards to testing and getting examples to work as @FoamyGuy was doing.

I agree it's worth having a broader discussion about checking the register. I didn't realize that was optional functionality for the device, and functions sort of like a configuration (using tachometer vs. checking for faults). It may be worth either having to different classes, or to have some kind of configuration setting in the __init__() method as @FoamyGuy suggested on his livestream. Essentially, it seems almost that there are two "use cases" for the device that slightly interfere with each other. I would say in that case the decision should be made at instancing the class what behavior should be the default in that case, and either of those options would do that; I would avoid having both be intermingled in the same class but am curious for other solutions as well.

One option might be to remove check_status from the EMC2101 class and move those calls to an override of the methods in the EMC2101_EXT class -- so the base class doesn't check by default but the EXT class can do so if you ask it to. Either way, should the default be to check or not?

This is also a good tandem solution with the above point. I think @FoamyGuy mentioned it on stream, but if I understand correctly, since fans may having some variations in input/output provided, we should be able to be flexible to that, while still allowing the most "basic" versions of those devices.

I agree that we'll need thorough documentation of the changes. Docstrings attached to new functionality should also help since they'll be available in Sphinx!

I would emphasise that the status register checks are not 'something is wrong with the code or the program, but that the hardware monitor is doing its job and telling you when the fan is too fast, too slow or the temp is too high or low for the situation. I fully agree that 'crashing' from an exception is not the right response. However, completely ignoring the status register isn't useful either.

I agree that crashing in this case doesn't make sense, since it's less of a "fault" and more of a "warning" - in essence, not strictly a failure. There may also be another to expose this functionality, but I can't think of any immediately. If only CircuitPython had the warnings module!

[rivimey]

Previous bunch of commits resulted from me rebasing the branch on the current main:HEAD, to avoid the previous conflict.

[FoamyGuy]

Personally I think the default behavior should be to not check status for the "main" driver classes currently used by the examples.

A subclass that does the checks like you mentioned would be fine, or even adding a strict parameter or similar to the main class initializer and only doing the check when that was set to True.

This way the existing examples can still run with a fan that doesn't provide the tach pin.

[FoamyGuy]

@tannewt @tekktrik and I discussed this a bit during the "In the weeds" section of the weekly meeting today: https://youtu.be/lvgmQF7CNo4?t=4262

The consensus we came to is we don't want the driver library to force this status checking and exception raising behavior on the user. Instead the user code can call check_status() as needed to check for and raise errors if they want that behavior in a given project.

Based on that feedback I think we can remove auto_check_status and all of the calls to self._check_status() from the the driver. If you'd like to show people how to use the status functionality you could add a new example script that calls check_status() and handles the errors that it can raise.

[FoamyGuy]

@rivimey Do you think you'll have time to remove those automatic check_status calls? No worries if not

I'll follow up next week and push a commit to do that if you don't have a chance to get to it.

I'll do one more round of testing after that, but I think we should be ready to merge pretty quick after that.

[rivimey]

Sorry not been well. What state is this in now? Did you notice that changes I made a while ago made checking the status reg optional, disabled by default, and not present at all for the base class?

[FoamyGuy]

@rivimey No worries. Thank you for checking in.

I had not noticed that default behavior was changed when I made the prior comment, but I did see that today when I sat down to wrap this one up.

There were a few other things failing the actions check:

A Makefile without any attribution. I removed it, if it's something you created and you want to add a licence header to it you could re-add though truthfully I'm not sure what it's for typically libraries don't have a make file like that in my experience, but it looks like your project has a build step that is compiling this library?

The docs build was failing due to not finding an import. I made a tweak in conf.py for that and resolved a few other issues where things didn't have the proper name for the file they were in.

I think actions should be sorted now.

I'm intending to re-test the latest version from this branch with hardware later this afternoon.

@tekktrik do any of the concerns you raised remain open? Everything I could see seems to be handled with the latest version but wanted to double check with you.

[tekktrik]

I can take another look but I think they were. I'll do a final sweep just to be sure. Thanks both of you!

[tekktrik]

I should have a final review with anything this week hopefully, but this weekend at the latest.

[tekktrik]

@FoamyGuy have you had a chance to look at this yet? What do you think of the remaining points?

[FoamyGuy]

@tekktrik I am in agreement with the remaining open items you mentioned. I do believe I tested this back around the time of my previous comment, but also thing it's worth another quick check after the remaining changes are made. I'll circle back and test it out with the hardware again once another commit is in with those.

[ghost]

Sorry to add an 'extraneous' comment to your amazing work. Cant wait to use this version as, yh, mega useful!!

Willing to beta test as a random 3rd party but no idea how to clone this PR version into a useable python module...

Appreciate this is likely the wrong place to ask/talk so no offence if you remove comment.

Just a keen 3rd party wanting to see this over the line!!

[FoamyGuy]

@omorgan831 Sorry, didn't notice this comment for a bit.

If you're still interested in testing this (or more generally any pr) but don't know how.

If you do it frequently and are comfortable with command line tools. Look into the official github CLI https://cli.github.com/

it makes checking out a PR branch very easy. Inside of a locally cloned repo this would check out the branch for this repo:

gh pr checkout 23

If you're less comfortable with command line you can do it by clicking the link at the top of this page that goes to the authors branch:

Once you're on the authors fork of the project click the green button to get the clone URL and clone the project as normal.

Inside the locally cloned project run this to checkout branch for the PR. Make note of the name of the branch in the PR page and checkout that branch with a command like this:

git checkout extra_registers

[rivimey]

Just updated my branch to include the latest from adafruit. I don't think I have messed up this update... There were merges in the area of __version__definition in init.py and also in emc2101_lut.py.

Would someone care to point out where it would be appropriate to add my name to the project as an author? ... conf.py?

[rivimey]

@tekktrik I have reinstated the code I did and thought I'd included but "got lost" somewhere, to do with _defer_update. Sorry for that.

I'm sorry but I am unwilling to make any more changes. This PR has already gone on a lot longer than I was happy with, given I do have other things to do.

The debug print statements were left commented out very intentionally - in building the hardware it is very useful to know which things aren't working right. I don't propose, therefore, to remove them.

I have intentionally split up the module so that those with limited memory can use only the base class in init.py while leaving those with more resources to exploit the rest of the code.

The makefile that was deleted from the branch on 6th June included the commands needed to compile the code to both Arm and Python bytecode and so to compare the installed library size, should that be needed to e.g. check installed library size.

I have not makde any changes to the docs build file. That line was added by foamyguy on 6th june.

[FoamyGuy]

Would someone care to point out where it would be appropriate to add my name to the project as an author? ... conf.py?

In the copyright string at the top of the code files that you worked on is good I think. Such as here: https://github.com/rivimey/Adafruit_CircuitPython_EMC2101/blob/dc4845b221632ccdd0859abd92917927ddf769b2/adafruit_emc2101/emc2101_lut.py#L2

It looks like you did find this and add your name, I didn't check all the files, but feel free to add it to any others that you worked on.

[tekktrik]

@FoamyGuy took a look at this on stream today and the remaining items are what he (and I) thought are open items. I think there's still a need to test this which I can look into, but if you'd like to have one of us (or another community member) continue this please let us know. Thanks for all your contributions so far!

[rivimey]

It has for a while now felt like each time I think comments are addressed someone pipes up with another thing to change, and/or they are not satisfied with anything but what they want, negating whatever experience of the situation I might have. For example, issues about code size are reasonable but (I thought) dealt with several months ago, and the str/repr functions were apparently fine until now. This might be ok when you're all working for the same org to the same goals, but I'm doing this in my "free" time, along with a lot of other projects.

Basically, the experience I've had here makes me less inclined to contribute anything major again because I know I'll be pushed to change the code in ways that don't fit my own goals & aspirations. Beware of perfect being the enemy of good!

What I would like now is for this PR to be accepted and, should the community feel the need, then other changes can be made on top in other PRs as required.

@tekktrik please also note https://github.com/rivimey/Adafruit_CircuitPython_TCA9544A.git, which I've discussed here https://forums.adafruit.com/viewtopic.php?t=194387

[tekktrik]

If you want to contribute the TCA9544A library, I would suggest the Community Bundle which is the typical place for non-Adafruit sponsored libraries, but if your intent is to having in the Adafruit Bundle, I would check with someone about explicitly putting it there, since that isn't typical as far as I know. The support page could work, or even the #circuitpython-dev channel in the Adafruit Discord where the rest of the devs will see things.

My suggestion is the Community Bundle - the process is relatively easy to add it as a submodule there, and that bundle gets shared as well just like the Adafruit one. Note that if it goes in the community bundle you would remove Adafruit from the repo name and library filename.

[FoamyGuy]

@rivimey

We apologize if you have found the review process to be frustrating. It is not our intention to come off as un-satisfied or negate your experience, goals, or aspirations.

We do understand that you've got your own goals and aspirations with this code / library change. You are certainly free and encouraged to use the modified library for your own purposes how ever you'd like.

When considering whether to add the changes to the officially published library for all of our users to then find and use it, we must also take into consideration the goals and aspirations of the project and community as a whole. It's possible these may not be in 100% alignment with any specific community member, but in that instance it's not meant affront to that person. It is simply the community and maintainers attempting their best to do what they feel is in the best interest of the project and community. Individual community members have no obligation to make these considerations when working with their own code or libraries released within the Community Bundle rather than the official Adafruit bundle. We feel it's within the best interest of the project and community that we do consider these things when making descisions about libraries in the official Adafruit bundle.

There are certain coding styles, conventions, and other aspects that we want to be consistent across all of our published libraries. It's also a living, evolving process. We're constantly discussing new ideas, changes to our processes and points of interest to focus on. There have been more recent discussions occuring in our community about library size. The mention of it here stemmed from some of the broader discussions that have been occuring recently.

The review process occurs in the open and everyone in the community is allowed and encouraged to participate. If there are changes that have been requested that someone does not agree with it's perfectly okay to have a (civil) discussion about pros and cons and for the community / project maintainers to make a desicion after understanding the different view points at hand. Tekktrik was not attacking you or your work with any items discussed or by mentioning that he has concerns about memory / size and that there might be some portions we want to leave out of the published library. We welcome discussion during the review process, if you or anyone doesn't agree with a request that was made, or a piece of feedback you can share your point of view and explain whether you agree / disagree and with which aspects of it. This process is not intended to be antagonistic in either direction (contributor or reviewer), it's a natural part of open source efforts.

Thank you for expanding/improving the functionality of this library. We appreciate the time and effort that you've put into it, I have no doubt folks will enjoy the new features as evidenced by some of the other comments on this PR.