Additional registers and register documentation, and round tach output to 2dp #23
Conversation
…t to 2dp. Lots of DP in tach reading is just a maths artifact.
|
I can see there was a problem with formatting, but not what the problem was. I may need help (or pointers) fixing it |
|
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 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!) |
|
Good to review now? |
adafruit_emc2101/__init__.py
Outdated
| @@ -255,7 +347,7 @@ def fan_speed(self): | |||
|
|
|||
| val = self._tach_read_lsb | |||
| val |= self._tach_read_msb << 8 | |||
| return _FAN_RPM_DIVISOR / val | |||
| return round(_FAN_RPM_DIVISOR / val, 2) | |||
There was a problem hiding this comment.
I don't know the use case for having the library use round() as opposed to usercode, is there a reason this should be done here? Is there any reason a user might want more precision?
If it should be rounded, the product tutorial has an example with 3 decimal places, is that better?
What are your thoughts @rivimey?
There was a problem hiding this comment.
The sensors have an accuracy, which is probably less than 1dp. The "additional" dp's present in the number are entirely mythical, arising from the division. I feel it is helpful to users not to present an apparent resolution far higher than the actual precision.
The reason for the low precision of the reading is partly that motors habitually fluctuate in speed around their nominal value, so any frequency measurement over time is an average; the pwm also has a resolution, and of course all these readings are in fact historical.
|
Also, thanks for adding all the registers, what a behemoth of a task! |
|
My only other concern is memory usage. Are the new registers accessed by the library? Adding this many |
|
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 :-) |
|
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 Wanna try keeping the "essential" functionality additions/changes as is, and adding more of the "extra" ones to the LUT class? |
Decided to create an intermediate non-LUT class, so people wanting the extended regs aren't forced into LUT mode. So we now have EMC2101_EXT. Moved all but the part_rev register into new class. Added (currently untested) get/set properties for int & ext temp limits using newly added regs.
…er; fix duplicated _pwm_freq_div
|
I have moved the registers around a bit, and I hope things are looking better now. |
…rn lut values as data not string
|
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 adafruit/Adafruit_Blinka#578 which is related? |
|
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! |
|
Bump? |
|
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 |
adafruit_emc2101/__init__.py
Outdated
| _fan_lut_prog = RWBit(EMC2101_Regs.FAN_CONFIG, 5) | ||
| """Programming-enable (write-enable) bit for the LUT registers.""" | ||
|
|
||
| _fan_temp_hyst = RWBits(5, EMC2101_Regs.FAN_TEMP_HYST, 0) |
There was a problem hiding this comment.
Should this be moved to the LUT class?
There was a problem hiding this comment.
fan_lut_prog is used by the non-lut class to report whether the lut is enabled (even if the sw lut code isn't there) because the lut must be disabled in order to manually change fan speed. I guess fan_temp_hyst could move?
| _fan_setting = UnaryStruct(_REG_FAN_SETTING, "<B") | ||
| _pwm_freq = RWBits(5, _PWM_FREQ, 0) | ||
| _fan_lut_prog = RWBit(_FAN_CONFIG, 5) | ||
| invert_fan_output = RWBit(_FAN_CONFIG, 4) |
There was a problem hiding this comment.
It looks like this was exposed, if we removing it, it;s technically breaking. I'd recommend adding back in if there's no explicit reason to remove it.
There was a problem hiding this comment.
ok, I'll move back. I was kind-of assuming that the reason for the _ prefix to these names was to remind users that they were internal, implementation-specific variables and thus not to be relied on.
However, I can move them if you wish.
There was a problem hiding this comment.
That's it's use, but it looks like it was removed entirely and was previously exposed, so it's possible someone was relying on it because it's exposed, but would now have their code break. Readding it will fix it though!
There was a problem hiding this comment.
This is in reference to invert_fan_output
There was a problem hiding this comment.
I have reinstated invert_fan_output now.
|
@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 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. |
|
I can take another look but I think they were. I'll do a final sweep just to be sure. Thanks both of you! |
|
I should have a final review with anything this week hopefully, but this weekend at the latest. |
| _fan_lut_prog = RWBit(emc2101_regs.FAN_CONFIG, 5) | ||
| """Programming-enable (write-enable) bit for the LUT registers.""" | ||
|
|
||
| _fan_temp_hyst = RWBits(5, emc2101_regs.FAN_TEMP_HYST, 0) |
There was a problem hiding this comment.
Should this be in the LUT class?
There was a problem hiding this comment.
I seem to remember (vaguely) this was in the original base class, and so didn't want to move it even though it was a "private". I believe you're right in saying it could be moved.
adafruit_emc2101/__init__.py
Outdated
|
|
||
| return full_tmp | ||
| full_tmp *= 0.125 | ||
| return float(full_tmp) |
There was a problem hiding this comment.
This use of float() can be removed since the previous line guarantees it's already float
adafruit_emc2101/emc2101_fanspeed.py
Outdated
|
|
||
| """ | ||
| # Use increment/decrement so nested with's are dealt with properly. | ||
| self._defer_update += 1 |
There was a problem hiding this comment.
I think the use of _defer_update to account for nested uses of with isn't needed since there shouldn't ever be a case where nested with for the same device should be done. This is also true for __exit__(). I also think it goes against the concept of the context manager if it doesn't perform the intended exit action upon leaving, so I would strongly advise on changing that behavior.
In essence, if someone is using nested with statements, it's probably more likely their code isn't working as they intended anyway, and it's better to fail so that can be addressed, rather than try to fix things in the background and mask other problems.
There was a problem hiding this comment.
I think regarding the use of allowing __setitem__() to work within and outside of a context manager, a bool internal variable would be more appropriate since it could hold the state only as long as the context manager is being used.
There was a problem hiding this comment.
This change has been made.
There was a problem hiding this comment.
It looks like this is still here in the proposed change right now, between here and __setitem__()
|
@FoamyGuy have you had a chance to look at this yet? What do you think of the remaining points? |
|
@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. |
|
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!! |
|
@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: 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: |
…Python_EMC2101 into extra_registers
…2101 into extra_registers
|
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 Would someone care to point out where it would be appropriate to add my name to the project as an author? ... conf.py? |
There was a problem hiding this comment.
Feedback below, sorry for the delay! I'm still a little worried about the memory change, but @FoamyGuy is working on a tool to help quantify that so we can better make judgements and adjustments. I think that would be really useful for this PR. For example, it may be worth optimizing the registers by placing them in the files they should be. This way we can use _varname = const(0x00) to make the most of the compiler optimizations (as opposed to varname = const(0x00)). @FoamyGuy let me know if I can help with that in any way.
adafruit_emc2101/emc2101_fanspeed.py
Outdated
|
|
||
| """ | ||
| # Use increment/decrement so nested with's are dealt with properly. | ||
| self._defer_update += 1 |
There was a problem hiding this comment.
It looks like this is still here in the proposed change right now, between here and __setitem__()
| temp *= 0.125 | ||
| if not -64 <= temp <= 127: | ||
| # This should be impossible, if it happens the i2c data is corrupted. | ||
| raise OSError("Connection") |
There was a problem hiding this comment.
If you think this will happen infrequently, you could just raise OSError, which will cut out a string in the RAM.
| full_tmp *= 0.125 | ||
| if not -64 <= full_tmp <= 127: | ||
| # This should be impossible, if it happens the i2c data is corrupted. | ||
| raise OSError("Connection") |
There was a problem hiding this comment.
Same thing here as before about leaving out the string.
|
@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. |
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. |
|
@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! |
|
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 |
|
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. |
|
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. |
There was a problem hiding this comment.
I have re-tested the basic PWM output functionality with latest version of this branch with
Adafruit CircuitPython 8.0.0-beta.0 on 2022-08-18; Adafruit Feather ESP32-S2 TFT with ESP32S2
Board ID:adafruit_feather_esp32s2_tft
Using a slightly modified simpletest. My fan has no tachometer or temperature feedback so I commented out the portions that deal with that.
PWM output of various different levels appears to me to be working identically in this PR branch version as it does in the current released version.
This looks good for merging to me now.
Any remaining changes we want to make can be done in a follow up PR(s). @tekktrik if you'd like to experiment with using the leading underscore for the constants and remove or change the error message strings go ahead. We can also try adding the mpy size check actions task at that time (after I get the main branch vs. release bit worked out in it). It could provide insight and quantification of the difference that compiler makes with it's handling of constants.
Updating https://github.com/adafruit/Adafruit_CircuitPython_EMC2101 to 1.2.0 from 1.1.17: > Merge pull request adafruit/Adafruit_CircuitPython_EMC2101#23 from rivimey/extra_registers
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:
I may work on this further. Let me know if there are things I need to know.