Skip to content

core: arduino: add ShiftIn/ShiftOut implementation #395

Open
KurtE wants to merge 1 commit intoarduino:mainfrom
KurtE:shiftIn_shiftOut
Open

core: arduino: add ShiftIn/ShiftOut implementation #395
KurtE wants to merge 1 commit intoarduino:mainfrom
KurtE:shiftIn_shiftOut

Conversation

@KurtE
Copy link

@KurtE KurtE commented Mar 16, 2026

resolves: #345

Note: I simply copied in the MBED version of this. The APIS were already contained within the header files.

Verified that the sketch within the issue built.
Modified it to verify that worked at all:

void setup() {
  // put your setup code here, to run once:
  pinMode(2, INPUT);
  pinMode(3, OUTPUT);
  pinMode(4, OUTPUT);
  pinMode(5, OUTPUT);
  //shiftIn(2, 3, MSBFIRST);
  //shiftOut(4, 5, LSBFIRST, 42);
  pinMode(LED_BUILTIN, OUTPUT);

}

uint8_t loop_count = 0;
void loop() {
  // put your main code here, to run repeatedly:
  loop_count++;
  digitalWrite(LED_BUILTIN, loop_count & 1);
  shiftOut(4, 5, LSBFIRST, loop_count);
  delay(1);
}

Built and ran on UnoQ with current sources as of this morning.
image

It has the same possible issue as the MBED version in that the faster the processor, the narrower the pulses are:
image
I know in the case of Teensy 4.x boards, they put in possible delays, but that could be looked at later if needed.

@per1234 per1234 added the bug Something isn't working label Mar 16, 2026
@pennam pennam force-pushed the shiftIn_shiftOut branch from b000b78 to b67d0f4 Compare March 16, 2026 16:44
@pennam pennam changed the title Copy MBed version of ShiftIn/ShiftOut core: arduino: add ShiftIn/ShiftOut implementation Mar 16, 2026
@pennam pennam force-pushed the shiftIn_shiftOut branch from b67d0f4 to f0289a9 Compare March 16, 2026 16:48
@pennam
Copy link

pennam commented Mar 16, 2026

Thanks @KurtE I've force pushed your changes to make the ci happy.

@KurtE KurtE force-pushed the shiftIn_shiftOut branch 2 times, most recently from c376871 to 569d86d Compare March 16, 2026 16:58
@KurtE
Copy link
Author

KurtE commented Mar 16, 2026

Thanks

Copy link

@pillo79 pillo79 left a comment

Choose a reason for hiding this comment

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

Thanks @KurtE, but we would like to avoid including LGPL licensed code directly in the main core. Even though the final license of any Arduino build is always LGPL, the "core" itself is Apache2 and this would be the first incompatible bit.

The implementation of shiftIn/shiftOut should be rewritten from scratch to make this viable for inclusion. Would you like to give it a try? 🤗

@KurtE
Copy link
Author

KurtE commented Mar 18, 2026

Thanks @KurtE, but we would like to avoid including LGPL licensed code directly in the main core. Even though the final license of any Arduino build is always LGPL, the "core" itself is Apache2 and this would be the first incompatible bit.

Sorry, not sure how things like that would apply, as for example my hands are dirty, so not sure what writing from
scratch would mean.

Maybe you could just copy in the version from renesas_uno, which does not say anything? So no idea what license that code has.

The complete shift.cpp

#include <Arduino.h>

uint8_t shiftIn(pin_size_t dataPin, uint8_t clockPin, BitOrder bitOrder) {
	uint8_t value = 0;
	uint8_t i;

	for (i = 0; i < 8; ++i) {
		digitalWrite(clockPin, HIGH);
		if (bitOrder == LSBFIRST)
			value |= digitalRead(dataPin) << i;
		else
			value |= digitalRead(dataPin) << (7 - i);
		digitalWrite(clockPin, LOW);
	}
	return value;
}

void shiftOut(pin_size_t dataPin, uint8_t clockPin, BitOrder bitOrder, uint8_t val)
{
	uint8_t i;

	for (i = 0; i < 8; i++)  {
		if (bitOrder == LSBFIRST)
			digitalWrite(dataPin, !!(val & (1 << i)) ? HIGH : LOW);
		else	
			digitalWrite(dataPin, !!(val & (1 << (7 - i))) ? HIGH : LOW);
			
		digitalWrite(clockPin, HIGH);
		digitalWrite(clockPin, LOW);		
	}
}

EDIT: Looks like that whole project has MIT License... Only one of your cores which are Apache2 are your
two (or three) Zephyr boards setup... Did that mean that for your zephyr cores all of the core had to be rewritten
from scratch?

@CLAassistant
Copy link

CLAassistant commented Mar 18, 2026

CLA assistant check
All committers have signed the CLA.

@KurtE KurtE closed this Mar 18, 2026
@KurtE KurtE reopened this Mar 20, 2026
@KurtE
Copy link
Author

KurtE commented Mar 20, 2026

I will see about hacking up a different version... Although hard to say I can come up with an original version 😉

@KurtE KurtE force-pushed the shiftIn_shiftOut branch 4 times, most recently from 163da03 to 6a04768 Compare March 20, 2026 21:31
@KurtE KurtE requested review from pennam and pillo79 March 20, 2026 21:32
@KurtE
Copy link
Author

KurtE commented Mar 20, 2026

I hacked in a different version of shiftIn and shiftOut,

The code is semi-similar to Teensy version, although the Teensy version is more advanced in that for example:
That is each of these two functions are made up of 3 or 4 routines. One for MSBFIRST one for LSBFirst, a version
in the header file that checks to see if what is passed in is a constant and not variable for (MSBFIRST/LSBFirst) and then it compiles it to call the appropriate ORDER function and the rest is optimized out, else it has full function that checks and calls the appropriate one. They also have checks in their code to see if it will be too fast and add in delays...
Actually for the output function, they only fully implement it for LSB order for MSB order they use the arm operator to reverse the bits and then call the LSB version...

This version does none of this, it simply checks MSB or LSB and has a for loop difference of either:
for (mask=1; mask; mask <<=1) or for (mask=-0x80; mask; mask >>=1)

Edit:
For testing of ShiftOut: I used the following sketch:

void setup() {
  pinMode(2, OUTPUT);
  pinMode(3, OUTPUT);
  pinMode(4, OUTPUT);
  pinMode(5, OUTPUT);
  pinMode(LED_BUILTIN, OUTPUT);
}

void loop() {
  digitalWrite(LED_BUILTIN, !digitalRead(LED_BUILTIN));
  for (uint16_t i = 0; i <= 0xff; i++) {
    shiftOut(2, 3, LSBFIRST, i);
    shiftOut(4, 5, MSBFIRST, i);
  }
  delay(250);
}

And checked with logic analyzer
image

For ShiftIn, I used the sketch:

//#define TEST_MSBFIRST
void setup() {
  Serial.begin(115200);
  pinMode(2, INPUT);
  pinMode(3, OUTPUT);
  pinMode(4, OUTPUT);

  pinMode(LED_BUILTIN, OUTPUT);

  attachInterrupt(3, &shiftOut_int, RISING);
}

uint8_t shiftOut_val;
void shiftOut_int() {
#ifdef TEST_MSBFIRST
  digitalWrite(4, (shiftOut_val & 0x80)? HIGH : LOW);
  shiftOut_val <<= 1;
#else  
  digitalWrite(4, (shiftOut_val & 0x01)? HIGH : LOW);
  shiftOut_val >>= 1;
#endif
}


void loop() {
  digitalWrite(LED_BUILTIN, !digitalRead(LED_BUILTIN));
  for (uint16_t i = 0; i <= 0xff; i++) {
    shiftOut_val = i;
#ifdef TEST_MSBFIRST
    uint8_t val = shiftIn(2, 3, MSBFIRST);
#else
    uint8_t val = shiftIn(2, 3, LSBFIRST);
#endif
    if (val != i) {
      printk("%x != %x\n", i, val);
      delay(250);
    }

  }
  delay(250);
}

With this I ran a jumper from D4 to D2 and checked for output Serial
And ran this on an Arduino GIGA

Copy link

@pillo79 pillo79 left a comment

Choose a reason for hiding this comment

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

Thank you for the code and test! I suggest one trivial fix, but LGTM! 🙂

@@ -0,0 +1,50 @@
/*
* Copyright (c) Arduino s.r.l. and/or its affiliated companies
Copy link

Choose a reason for hiding this comment

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

Use your name (or alias, if you prefer)... it's your work! 😉

Copy link
Author

Choose a reason for hiding this comment

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

Could do, but is there any way to mark it as Copyright, everyone who has ever shifted in or out a byte ;)

Copy link
Author

Choose a reason for hiding this comment

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

But pushed up the change

Copy link
Author

Choose a reason for hiding this comment

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

However now the test for licenses fail...

@KurtE KurtE force-pushed the shiftIn_shiftOut branch from 6a04768 to b32660d Compare March 25, 2026 14:44
@@ -0,0 +1,50 @@
/*
* Copyright (c) KurtE
Copy link

@pillo79 pillo79 Mar 25, 2026

Choose a reason for hiding this comment

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

It seems the name is too short for it to be reliably detected by scancode. Black magic.
Can you please try this alternative? (the tag is case sensitive)

Suggested change
* Copyright (c) KurtE
* SPDX-FileCopyrightText: Copyright (c) KurtE

(or try adding the year?).

@KurtE KurtE force-pushed the shiftIn_shiftOut branch from 68bf22d to 119b2a3 Compare March 25, 2026 15:33
@@ -0,0 +1,49 @@
/*
* SPDX-FileCopyrightText: Copyright (c) KurtE
Copy link

Choose a reason for hiding this comment

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

Did a few tests locally. It appears if you add SPDX-FileCopyrightText: then you do NOT have to add Copyright. But it does require the year. So confusing! 🤷‍♂️

I have tested this works:

 * Copyright (c) 2026 KurtE

(there is also * SPDX-FileCopyrightText: 2026 KurtE, but it isn't used anywhere else in the repo, so I'd prefer the above one).
Sorry about this flurry of changes!

Copy link
Author

Choose a reason for hiding this comment

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

Updated, fingers crossed

ShiftIn and ShiftOut different than MBED version.
@KurtE KurtE force-pushed the shiftIn_shiftOut branch from 119b2a3 to 62a9701 Compare March 25, 2026 16:09
@github-actions
Copy link

Built 0.54.1-0.dev+62a97010

CI run PASSED 🟢

ArtifactBoardCoreTestsRAMSketchesWarningsErrors
zephyr_contrib ek_ra8d1 📗

11.9%

2--
frdm_mcxn947 3 🏷️

58.0%

2--
frdm_rw612 1 🏷️

83.0%

2--
✔️* zephyr_main giga 4 🏷️ ✅*

54.5%

448-
nano33ble 1 🏷️ ✅*

78.7%

228-
nano_matter 📗 ✔️*

⚠️ 85.7%

208(2*)
niclasense 2 🏷️ ✅*

⚠️ 87.3%

208-
opta 4 🏷️ ✔️*

46.7%

548(2*)
portentac33 3 🏷️ ✔️*

⚠️ 95.1%

568(8*)
portentah7 3 🏷️ ✔️*

47.3%

588(2*)
✅* zephyr_unoq unoq 📗 ✅*

26.4%

628-
Legend

BoardTestStatus description
🔥 🔥 Test run failed to complete.
🔴 Test completed with unexpected errors.
✔️* 🚫 Test completed with errors, but all are known/expected.
✅* 🟡 Test completed with some warnings; no errors detected.
🟢 Test passed successfully, with no warnings or errors.
🌑 🌑 Test was skipped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

shiftIn + shiftOut not implemented

5 participants