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

drivers/cc110x: remove irq_disable/irq_restore around spi transfers #10670

Merged
merged 1 commit into from
Jan 7, 2019

Conversation

vincent-d
Copy link
Member

Contribution description

Same as #10669. I suspect cc110x has been used as a base for sx127x.

Testing procedure

Did not test as I don't have the hardware.

Issues/PRs references

#10669 #9171

@vincent-d vincent-d added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 28, 2018
@aabadie aabadie requested a review from maribu December 28, 2018 09:18
@aabadie
Copy link
Contributor

aabadie commented Dec 28, 2018

Asking @maribu for a review because I think he has access to that hardware.

@maribu
Copy link
Member

maribu commented Dec 28, 2018

I'm hoping to replace the cc110x by a complete rewrite, see #10340. I would suggest to not spend any effort in the current cc110x driver now. If my PR is not going to be merged, any fix to the current driver is still welcome. But I strongly believe that (maybe after several review-fix iterations) the rewrite of the cc110x driver will eventually be accepted and merged.

Update: OK, this change is not that much work to test. I changed my mind :-)

@maribu
Copy link
Member

maribu commented Dec 28, 2018

OK, code wise this should not be a problem. The driver has a lock on the SPI interface anyway, so there is no harm in keeping interrupts enabled - unless I overlook anything. I will not be able to test it until the 7th of January.

@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Dec 28, 2018
@aabadie
Copy link
Contributor

aabadie commented Dec 28, 2018

I will not be able to test it until the 7th of January.

No problem, we can wait a bit :)

@miri64
Copy link
Member

miri64 commented Dec 28, 2018

@maribu so you still would recommend to merge? (having fixes for old versions of a module might still be case in the [hopefully unlikely] case someone needs to revert to the old state).

@maribu
Copy link
Member

maribu commented Dec 28, 2018

@miri64: In general I would recommend to put effort on the current cc110x on pause, as

  1. There seem to be only me and @Citrullin left actively using those devices (please correct me if I'm wrong) - so effort seems to be better invested elsewhere in RIOT
  2. In case the rewrite gets merged (and not rolled back) that effort is likely to be to no avail. (Considering that I'm willing to work hard to address any issue with the rewrite and keep it maintained, I think it is likely to get (likely after several rounds of improvements) merged and unlikely to be rolled back.)

If I fail to get the rewrite up to state that can be merged, any effort on the current cc110x implementation can be just resumed.

In this particular case I would be in favor of merging (after I did the testing), as the effort to write the PR has already been done and testing it will be quite easy.

(Regarding the user base of the CC110x: That will increase by at least 900% (so up to twenty or more users ;-D) when/if the rewrite gets merged. Handing out a Bluepill, a CC1101 breakout board and a TTL adapter to students seems to be nice idea, as the possibility of them breaking or not returning hardware worth less than 5€ doesn't give me bad dreams. Also, we (ComSys) are currently looking into adding CC1101 transceivers (in addition to less ancient transceivers) to the nodes of our future testbed. Having additional transceivers operating at 433MHz - which seems to be almost unused in the area of the university - would have great value for experiments where external interference needs to be as little as possible.)

@Citrullin
Copy link
Contributor

@maribu @miri64 I am not using the old driver. I remember that the old driver was somehow complicated to use, so I stopped trying to us it. The new one is way simpler to configure. So I am going to use this one. Eventually I am trying to use it for commercial purposes.

I have to agree that the CC1101 is a really good device for educational purposes, since it is a simple RF protocol without encryption. BLE, Wifi is way more complicated to understand. Another point is commercial use-cases. The 868 band has a duty cycle of 1%, so it is only suitable for sensor-data, but not for package based communication. So, we could use Wifi, BLE, Zigbee etc. on the 2.4 Ghz band. As we all know this band is crowded in bigger cities. 2.4 Ghz on bigger events is also a problem. Therefore there are use-cases where the 433 band is more reliable than the 2.4 Ghz band.

@maribu
Copy link
Member

maribu commented Dec 29, 2018

@Citrullin:

The 868 band has a duty cycle of 1%, so it is only suitable for sensor-data

This applies to TX power of +14dBm without an upper bound on channel bandwidth. If you would limit TX power to +10dBm and channel bandwidth to < 25 kHz, you only need an appropriate access protocol. If you limit TX power further to +7dBm you do not need to obey limits on duty cycle or channel bandwidth within 869.7 - 870 MHz. See Table 7 in the ISM-Band and Short Range Device Regulatory Compliance Overview that TI kindly published. (Unlike the official regulatory documents, this document is quite understandable 😄)

So 868 MHz might be possible for your use case as well. (However, if you stick to the CC1101, I would still recommend the 433 MHz breakout boards. They are cheaper, easier to obtain and have better quality than the few 868 MHz boards.)

@Citrullin
Copy link
Contributor

@maribu I had Lora on 868 in mind, since it is common to use it on these frequencies. Nevermind, interesting to know that you can also use 868 without duty cycles. Probably true, since China only has 433 as I remember. As I remember 868 is for EU only. So that makes sense that the 433 modules are cheaper, since you can produce them in a larger quantity. Sounds to me like more good arguments for the 433 band when you want to have reliable short distance communication in bigger cities. But I guess there are also industries where the 433 is crowded as well.

I looked it up, I found 3 competitive modules for 433/868 (not lora):
RFM69HW, CC1101, NRF905
All in a similar price range. Btw: I started using RIOT because of the CC101 driver :D

@PeterKietzmann
Copy link
Member

I would almost merge this PR blindly and focus on #10340. Anyway, @maribu today is the 7th, could you give it a quick shot so we get this off the table?

@maribu maribu added Reviewed: 3-testing The PR was tested according to the maintainer guidelines CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 7, 2019
@maribu
Copy link
Member

maribu commented Jan 7, 2019

Tested and works. Lets wait for Murdock and merge.

(Note: I tested with the MSB-A2, which - if I remember correctly - does not have any SPI device other than the CC1100 on the same bus. If this PR would introduce any regressions, I would expect those when two drivers try to use the same SPI bus at the same time.)

@aabadie
Copy link
Contributor

aabadie commented Jan 7, 2019

Tested and works. Lets wait for Murdock and merge.

Very nice, thanks for testing @maribu!

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Murdock is green, so let's go!

ACK

@aabadie aabadie merged commit 4365d66 into RIOT-OS:master Jan 7, 2019
@PeterKietzmann
Copy link
Member

thx

@aabadie aabadie added this to the Release 2019.01 milestone Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants