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

Small fix on BME280 Config & CC1101-TXPower Throttle #1382

Merged
merged 6 commits into from
Jan 10, 2023
Merged

Small fix on BME280 Config & CC1101-TXPower Throttle #1382

merged 6 commits into from
Jan 10, 2023

Conversation

Dattel
Copy link
Contributor

@Dattel Dattel commented Jan 8, 2023

Description:

see discussion on:
#1380

Sorry, it's my first pull request and i mixed in another small change on BME280 configuration to make it possible to define BME Pins in platformio.ini

Checklist:

  • The pull request is done against the latest development branch
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • I accept the DCO.

Dattel added 2 commits January 8, 2023 12:57
…piler switch from the platformio.ini

Sample:
  '-DBME280_PIN_SDA=0'
  '-DBME280_PIN_SCL=2'
@NorthernMan54
Copy link
Collaborator

@Dattel Tks for this pull request, it looks really good

Can you make the following tweaks and update the pull request

1 - Pls fix the lint issue reported by the automated checks

2 - Can you add the new parameter to the documentation for the RF Module. The documentation is located here https://github.com/1technophile/OpenMQTTGateway/blob/development/docs/use/rf.md#rcswitch-based-gateway and is part of the repository, so please include the update as part of the pull request.

3 - I watching the conversation in #1380, and do we need the feature ?

Tks

@Dattel
Copy link
Contributor Author

Dattel commented Jan 10, 2023

Hi @NorthernMan54,
it's up to you to decide, if that's a usefull feature to use in OmqttG.
From my side it's a suggestion, since i was recently confronted with the problem. I like the flexibility to define a power-value for each sended RF-Code since only 2 key's of my remote seems to collide with "foreign" Devices :-D

I will have a look on the lint-error's but i have no clue, whats's really the problem. I guess the indent was not right?

@NorthernMan54
Copy link
Collaborator

To find the lint issues, I go to the checks section of the pull request and look at the details for the failed check. The lint check is pretty good as it shows you the fix.

In regards to the CC1101-TXPower Throttle feature, if you or @DigiH were going to make use of the feature then lets add it, but adding a feature/code that is never going to be used turns into a future code maintenance issue.

@DigiH
Copy link
Collaborator

DigiH commented Jan 10, 2023

In regards to the CC1101-TXPower Throttle feature, if you or @DigiH were going to make use of the feature then lets add it, but adding a feature/code that is never going to be used turns into a future code maintenance issue.

I'm afraid I'm on the give me a signal as strong as possible side - so this would not be useful for me personally.

@Dattel
Copy link
Contributor Author

Dattel commented Jan 10, 2023

Okay, i'm with you "strongest TX" will match almost 99% of all usecases. That's the reason for the paramateter to be as much optional as possible. And i'm also with you that every rare used feature is a possible source of error in case of refactoring...

So in case, these implementation is declined, i would make me a stash to apply on every new version because i already use it to avoid neighborhood disputes :-D
But the argument "I don't use it, so we don't adapt it" would be a very weak reason - then you could certainly question some other features as well :-D

@DigiH
Copy link
Collaborator

DigiH commented Jan 10, 2023

👍 Thanks @Dattel.

Could you still add the documentation about the new parameter, as requested above by @NorthernMan54?
Then this will be ready for merging.

@DigiH
Copy link
Collaborator

DigiH commented Jan 10, 2023

Sorry for being awkward here, but since this is a CC1101 only feature could you move the information down a bit to the section
Set Transmit and Receive Frequency of CC1101 Transceiver Module
rename it to
Set Transmit and Receive Frequency and Transmit Power of CC1101 Transceiver
and include your information there.
Currently, apart from the actual key name, it is included in the general RF options also applicable to other transmitters.

@Dattel
Copy link
Contributor Author

Dattel commented Jan 10, 2023

you are right :-D

@DigiH
Copy link
Collaborator

DigiH commented Jan 10, 2023

Looking good, thanks.

Congratulations on your first PR 🎉

@DigiH DigiH merged commit 5fcf02a into 1technophile:development Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants