Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSS811 baseline support #10858

Closed
wants to merge 2 commits into from

Conversation

clanganke
Copy link

@clanganke clanganke commented Feb 6, 2021

Description:

Added CCS811 baseline and multiple device support and commands to read both firmware application and hardware revision.
Tests run with ESP8266 and ESP32 core with one and two sensors and with one and two 7seg displays, see
https://www.clanganke.de/tasmota_pr/2021_02_pr_ccs811_update.zip
for more testcase details, screenshots and photos.
Sorry, misspelled the sensor name in the request title and the commit message

**Related issue (if applicable): none

Checklist:

  • The pull request is done against the latest development branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works on Tasmota core ESP8266 V.2.7.4.9
  • The code change is tested and works on Tasmota core ESP32 V.1.0.5-rc6
  • I accept the CLA.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

@arendst
Copy link
Owner

arendst commented Feb 7, 2021

Thx.

Tasmota contains a lot of function to be used to handle commands etc. I see you have just implemented the same again increasing code size using function that can be handled by already present functions.

I suggest you revisit the commands section looking for how to use function DecodeCommand. The present S_JSON_CCS811... conastants should not be needed.

If you want to use commands you will need to use a prefix too as a command like HW is likely to interfere with other (future) command. I suggest you select prefix Ccs. Again look for an example at the BH1750 driver.

@arendst arendst added the on hold by dev team Result - Feature request put on hold by member of development team label Feb 7, 2021
@clanganke
Copy link
Author

Thnx for viewing.

Just checked out BH1750 and now remember that scheme from the 7segment driver I did a pr recently for, however for the sensor I wasn't aware of that it could be or better had to be used there as well. I just took an existing driver as a template (here: for the SCD30 as I own this as well), not realizing that this was kind of 'outdated' coding style. Will be easy to change.

Moreover, sory to say that but I have no idea what or where "the command section" is, describing the DecodeCommand function. Maybe I have missed sth obvious, but I checked the contributing page in the wiki as well as the projects Readme.md on github, but could not find pointer to code related documentation there. I would greatly appreciate any pointer to such (would possibly have made code digging and understanding a lot easier).

One thing about the prefix: You suggested CCS asd command prefix, however I wonder if the full name of the chip - here: CS811 - is not still the best prefix for device specific commands, just in case any other sensor would later be named sth like CCS* as well. Possibly you meant the same?!

BTW, who would be the someone to discuss lager code changes with and what would be the favourite place or method for this? I have just finished implementation for support of the "Adafruit 128x64 OLED FeatherWing" (https://learn.adafruit.com/adafruit-128x64-oled-featherwing). For a new req. base class in the Adafruit GFX library this one and as well the BaseIO Library required updates, of course with the 'gemu' changes'. I also implemented an upate to the SSD1306 library, although this seems not absolutely to be neccessary. If that was a welcomed contribution to the project, I'd had more details and more questions on this.

@arendst
Copy link
Owner

arendst commented Feb 8, 2021

the command section

I meant your code providing current command support.

CCS811 as prefix is fine.

I prefer small tested changes so if you want to change display libraries make sure the steps are tested (or deduced tested) for all current supported displays.

@clanganke
Copy link
Author

I have just tried to rebase the pr - since I am rather new to git and pull requests and could not find any tutorial that I would fully understand, I can only hope that I did not screw up sth - I simply don't know.

Concerning the other change I proposed above I have got more details to tell, but would rather not bury such information in this conversation for a completely distinct pull request. So can we discuss this elsewhere? What would be the best place to do so?

@github-actions
Copy link

This PR has been automatically marked as stale because it hasn't any activity in last few weeks. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Action - Issue left behind - Used by the BOT to call for attention label Mar 17, 2021
@arendst arendst removed the stale Action - Issue left behind - Used by the BOT to call for attention label Mar 19, 2021
@arendst arendst self-assigned this Mar 31, 2021
arendst added a commit that referenced this pull request Mar 31, 2021
Add support for multiple CCS811 sensors with baseline control (USE_CCS811_V2) by clanganke (#10858)
@arendst
Copy link
Owner

arendst commented Mar 31, 2021

Thx. Merged as define USE_CCS811_V2

@arendst arendst closed this Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold by dev team Result - Feature request put on hold by member of development team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants