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

libtock: Rewrite and standardize API #370

Merged
merged 147 commits into from
May 8, 2024
Merged

libtock: Rewrite and standardize API #370

merged 147 commits into from
May 8, 2024

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented Feb 9, 2024

Rewrite libtock-c with a standardized format. See the guide.md file.

Progress

Apps

  • tests/ieee802154
    • tests/ieee802154/radio_tx
    • tests/ieee802154/radio_ack
    • tests/ieee802154/radio_rxtx
    • tests/ieee802154/radio_rx
  • tests/udp
    • tests/udp/udp_virt_app_kernel
    • tests/udp/udp_virt_rx_tests/app2
    • tests/udp/udp_virt_rx_tests/app1
    • tests/udp/udp_send
    • tests/udp/udp_rx
    • tests/udp/udp_virt_app_tests/app2
    • tests/udp/udp_virt_app_tests/app1
  • courses/2018-11-SenSys/sensys_udp_rx
  • courses/2018-11-SenSys/important-client/app2
  • courses/2018-11-SenSys/important-client/app1
  • ip_sense
  • tests/i2c
    • tests/i2c/i2c_master_ping_pong
    • tests/i2c/i2c_master_slave_ping_pong
  • tutorials/03_ble_scan
  • tests/imix
  • ./tests/sdcard
  • ./tests/adc_continuous
  • ./tests/hail
  • ./tests/adc_single_samples
  • ./tests/adc
  • ./tests/microbit_v2
  • ./adc
  • lvgl
  • music
  • witenergy
  • unit_tests/nrf52840dk-test
  • touch
  • tests/lsm303dlhc
  • lora/sensor-transmit
  • lora/sensor-receive
  • find_north
  • rtc_app
  • services/ble-env-sense
  • services/ble-env-sense/test-with-sensors
  • tutorials/05_ipc/led
  • tutorials/05_ipc/rng
  • tutorials/02_button_print
  • tutorials/hotp
    • tutorials/hotp/hotp_starter
    • tutorials/hotp/hotp_oracle_complete
    • tutorials/hotp/hotp_milestone_three
    • tutorials/hotp/hotp_milestone_two
    • tutorials/hotp/hotp_milestone_one
  • screen
  • tests/aes
  • security_app
  • ble-uart
  • tests/multi_alarm_test
  • tests/analog_comparator
  • tests/app_state
  • tests/touch
  • tests/crc
  • tests/hail
  • tests/hmac
  • ./tests/ble/ble_nrf5x_concurrency/Advertiser1
  • ./tests/ble/ble_nrf5x_concurrency/Advertiser3
  • ./tests/ble/ble_nrf5x_concurrency/Advertiser4
  • ./tests/ble/ble_nrf5x_concurrency/Advertiser2
  • ./tests/ble/ble_advertise
  • ./tests/ble/ble_test_tx_power
  • ./tests/ble/ble_gap_library
  • ./tests/u8g2-demos/led-menu
  • ./tests/console_timeout
  • ./tests/number_guess_game
  • ./tests/screen-tock
  • ./tests/console

@brghena
Copy link
Contributor

brghena commented Feb 9, 2024

I like this idea

@bradjc bradjc force-pushed the libtock-folders branch 2 times, most recently from c7d056a to 2ffda98 Compare February 9, 2024 21:49
@bradjc
Copy link
Contributor Author

bradjc commented Feb 9, 2024

Note: I don't know where to put crc. In the kernel syscalls its under "Cryptography", but that seems not right.

doc/guide.md Outdated Show resolved Hide resolved
doc/guide.md Outdated Show resolved Hide resolved
@bradjc
Copy link
Contributor Author

bradjc commented Feb 9, 2024

I've gone through and updated temperature and button to match the guide I wrote.

@bradjc bradjc changed the title libtock: Organize into directories libtock: Organize into directories and standardize driver format Feb 13, 2024
doc/guide.md Outdated Show resolved Hide resolved
doc/guide.md Outdated Show resolved Hide resolved
@tyler-potyondy
Copy link
Contributor

@bradjc this looks great and does an excellent job formalizing the convention in guide.md. I'm in favor of these changes and think it would mark a good step forward towards having more accessible documentation.

Copy link
Contributor

@brghena brghena left a comment

Choose a reason for hiding this comment

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

This represents a significant improvement to libtock-c and I strongly support it.

I read through all of the guide and I'm for it. We can bikeshed some names for a bit, but the plan it lays out resonates with me.

doc/guide.md Outdated Show resolved Hide resolved
doc/guide.md Outdated Show resolved Hide resolved
doc/guide.md Outdated Show resolved Hide resolved
doc/guide.md Outdated Show resolved Hide resolved
@bradjc
Copy link
Contributor Author

bradjc commented Feb 16, 2024

Couple things:

  1. Should we namespace libtock functions with libtock_?
  2. Should we put all header files in #include <libtock/sensors/temperature.h>? If so, how?
  3. We probably need to specify the cpp wrapper for the header files.

@tyler-potyondy
Copy link
Contributor

1. Should we namespace libtock functions with `libtock_`?

I am in support of this.

2. Should we put all header files in `#include <libtock/sensors/temperature.h>`? If so, how?

3. We probably need to specify the cpp wrapper for the header files.

Could you provide some more context with this. I'm not sure I follow with what this would be / how it would look.

@tyler-potyondy
Copy link
Contributor

One other thought for the guide file:

There are some kernel assumptions regarding allow buffers. Although this is documented in the TRDs, I think we should provide a streamlined summary of how these buffers should and should not be used in this guide.

For instance (copied from the the syscall TRD):

The standard access model for allowed buffers is that userspace does
not read or write a buffer that has been allowed: access to the memory
is intended to be exclusive either to userspace or to the kernel. To
regain access to a passed buffer B, the process calls the same
Read-Write Allow system call again. If this call returns a success
result, the result contains buffer B. The process can call with a zero-length
buffer if it wishes to pass no memory to the kernel. Once a buffer has been returned to userspace as
part of a Read-Write Allow system call, it is guaranteed for the
kernel to no longer have access to the described memory region, unless
it is currently shared with the kernel as part of the passed in buffer
or another Allow mechanism.

Expecting users to call the allow_readwrite function again to regain control of the buffer seems somewhat clunky from the perspective of userspace. I think a wrapper within libtock-c may be worthwhile here (perhaps an unallow_readwrite or disallow_readwrite function).

Regardless, we should codify the expected convention and use in simple terms for this guide.

@bradjc
Copy link
Contributor Author

bradjc commented Feb 20, 2024

What about putting all header files in libtock/include/libtock/[category]/? Then our apps would do #include <libtock/interface/led.h>.

This is what zephyr does: https://github.com/zephyrproject-rtos/zephyr/tree/main/include/zephyr

@alistair23
Copy link
Contributor

The current method of including headers causes conflicts with other libraries. For example #include <alarm.h> is pretty common.

So putting it under libtock would be a great idea (the categories also help fix this issue)

@brghena
Copy link
Contributor

brghena commented Feb 22, 2024

While working on the redesign: is this the time to approach how to handle multiple instantiations of a single syscall driver?

Came up again in tock/tock#3867

This could be handled with a struct for each driver that encapsulates its instantiated syscall number. Or it could be handled in the build system possibly, allowing us to include duplicate copies of a C file with a different #define value and name?

Or we could keep punting on it as an orthogonal problem that mostly occurs for downstream users (who could just make a duplicate C file with a different name if they wanted to).

@bradjc
Copy link
Contributor Author

bradjc commented Feb 23, 2024

This could be handled with a struct for each driver that encapsulates its instantiated syscall number. Or it could be handled in the build system possibly, allowing us to include duplicate copies of a C file with a different #define value and name?

In these approaches, how would we default to what we have now? It seems like either we would have to add additional code in the app to choose the driver numbers, or apps could configure themselves in a makefile and completely change what they are doing with no changes to code. Neither seems desirable.

@bradjc bradjc force-pushed the libtock-folders branch 2 times, most recently from a46280f to c57a4c8 Compare March 9, 2024 16:36
doc/guide.md Show resolved Hide resolved
@bradjc
Copy link
Contributor Author

bradjc commented Mar 23, 2024

We need to figure out an allow buffer management strategy. What layer is responsible for un-allowing the buffer? Are buffers passed back in callbacks? Or should users of the library hold their own copy of the pointer to the buffer?

@bradjc
Copy link
Contributor Author

bradjc commented Mar 26, 2024

The unit test framework stores state. Where do we put it?

@bradjc bradjc changed the title libtock: Organize into directories and standardize driver format libtock: Rewrite and standardize API Mar 27, 2024
@bradjc
Copy link
Contributor Author

bradjc commented Mar 29, 2024

Three more issues that have come up:

  1. We need to use more of status_to_returncode
  2. Should the upcall format be int? Why not just uint32_t?
  3. Should we document that all tock.h functions start with tock_? What about subscribe(), etc.?

@bradjc
Copy link
Contributor Author

bradjc commented Mar 29, 2024

I would also like to revisit what goes in examples/ and what goes in examples/tests.

@bradjc
Copy link
Contributor Author

bradjc commented May 7, 2024

I'm ready to merge this.

@ppannuto ppannuto dismissed tyler-potyondy’s stale review May 7, 2024 19:24

stale review, changes addressed

Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

This is massive, and in pretty good shape / clean at the moment. I think we should merge this, and then if there's anything missing we can clean it up with patches later. I can't see this getting much better than it is in the short term.

@ppannuto
Copy link
Member

ppannuto commented May 8, 2024

Pushed a tag for users who would like to defer updating to the new API here: https://github.com/tock/libtock-c/releases/tag/2.x-legacy-api — merging!

@ppannuto ppannuto added this pull request to the merge queue May 8, 2024
Merged via the queue into master with commit b254eb4 May 8, 2024
4 checks passed
@ppannuto ppannuto deleted the libtock-folders branch May 8, 2024 00:18
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.

8 participants